Skip to content

Test persistent! #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Test persistent! #794

wants to merge 1 commit into from
+57 −4

Conversation

@super7ramp
Copy link
Contributor

@super7ramp super7ramp commented Nov 10, 2025

This closes #394.

E-A-Griffin
E-A-Griffin reviewed Nov 10, 2025
View reviewed changes
Copy link
Collaborator

@E-A-Griffin E-A-Griffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test that transient operations (e.g. conj!) throw an Exception after calling persistent!, since the docstring mentions this as behavior?

test/clojure/core_test/assoc_bang.cljc Outdated
@@ -1,5 +1,5 @@
(ns clojure.core-test.assoc-bang
(:require [clojure.test :refer [deftest testing are]]
(:require [clojure.test :refer [are deftest testing is]]
Copy link
Collaborator

@E-A-Griffin E-A-Griffin Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be alphabetic in the template, I'll push up a fix for this

@super7ramp
Copy link
Contributor Author

super7ramp commented Nov 10, 2025 •
edited
Loading

Can you test that transient operations (e.g. conj!) throw an Exception after calling persistent!, since the docstring mentions this as behavior?

I added cases for this behavior in the tests of the transient operations. See in this PR disj! and assoc!, I'm going to add dissoc! that I forgot. conj! and pop! are not tested yet.

EDIT - added case for dissoc!

@super7ramp super7ramp force-pushed the persistent-bang branch 2 times, most recently from 76624de to 97f18a5 Compare November 10, 2025 19:14
jeaye
jeaye requested changes Nov 17, 2025
View reviewed changes
Copy link
Member

@jeaye jeaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

test/clojure/core_test/persistent_bang.cljc
#{} (transient #{})
#{nil} (transient #{nil})
#{:a :b :c} (transient #{:a :b :c})))

Copy link
Member

@jeaye jeaye Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want a test for calling persistent! multiple times on a transient, since that will throw. We don't need to do it for every possible shape, but we do want to capture that behavior, since it's documented.

Copy link
Contributor Author

@super7ramp super7ramp Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added case for this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeaye jeaye jeaye requested changes

+1 more reviewer

@E-A-Griffin E-A-Griffin E-A-Griffin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clojure.core/persistent!

3 participants

@super7ramp @jeaye @E-A-Griffin