Skip to content
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.

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

[minor] Add failing test for #819 #820

Conversation

chrissantamaria
Copy link
Contributor

See #819

@mweststrate
Copy link
Collaborator

Thanks for PR the test. Would you mind adding the other test case in your report as well, as I'd expect this PR to fail without fix :)

@chrissantamaria
Copy link
Contributor Author

Sure! Can you clarify which one you're referring to? I included this in my report but it's just a more specific variation of the original test:

it("maintains order when adding", () => {
  const objs = [
    {
      id: "a"
    },
    "b"
  ]

  const set = new Set([objs[0]])
  const newSet = produce(set, draft => {
    draft.add(objs[1])
  })

  // does not pass
  expect(Array.from(newSet)).toEqual([objs[0], objs[1]])
})

This branch should already be failing with yarn test

@mweststrate
Copy link
Collaborator

@chrissantamaria yeah it'd be great to add that one as well. I had expected this branch to fail, but it seems the CI didn't run at all in the first place, which confusingly is represented as all green it seems.

@chrissantamaria
Copy link
Contributor Author

haha yeah I noticed that, looks like Travis stopped checking commits a few weeks ago

Would you prefer all of these to be in base.js or map-set.js? I noticed the latter has some tests specifically labeled by issue #, happy to follow that standard if you'd like

@mweststrate
Copy link
Collaborator

Preferably map-set, base is a bit overcrowded :-P.

Well, I guess we have to migrate to GH actions finally.

@chrissantamaria
Copy link
Contributor Author

Done - wasn't quite sure how to differentiate those tests in the naming so feel free to edit

Happy to take a look at GH Actions integration if you want :) might be worth filing an issue regardless

@mweststrate
Copy link
Collaborator

Done - wasn't quite sure how to differentiate those tests in the naming so feel free to edit

Looking good!

Happy to take a look at GH Actions integration if you want :) might be worth filing an issue regardless

If you'd be interested in taking a stab at it that would be brilliant :). Otherwise I'll try to find some time next week

@chrissantamaria chrissantamaria force-pushed the failing-test-set-maintains-order-when-adding branch from faf9464 to 38fa33c Compare July 5, 2021 17:08
@netlify
Copy link

netlify bot commented Jul 5, 2021

✔️ Deploy Preview for quizzical-lovelace-dcbd6a canceled.

🔨 Explore the source changes: 38fa33c

🔍 Inspect the deploy log: https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/60e33c911d29f00007f13f5a

@mweststrate
Copy link
Collaborator

Retest this PR after merging #976

@mweststrate mweststrate changed the title Add failing test for #819 [minor] Add failing test for #819 Jan 15, 2023
@mweststrate mweststrate merged commit 847b662 into immerjs:main Jan 15, 2023
@chrissantamaria chrissantamaria deleted the failing-test-set-maintains-order-when-adding branch January 15, 2023 18:28
@github-actions
Copy link
Contributor

🎉 This PR is included in version 9.0.19 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants