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

Fixups: post-merge updates related to PR #6241 #6335

Merged
merged 6 commits into from
Aug 24, 2024
Merged

Fixups: post-merge updates related to PR #6241 #6335

merged 6 commits into from
Aug 24, 2024

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Aug 23, 2024

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

If this is related to an existing ticket, include a link to it as well.

Relates-to / merge-resolution-for #6241 and #6334.

Edit: add additional fixup descriptions.

@jayaddison

This comment was marked as outdated.

@jayaddison

This comment was marked as outdated.

@jayaddison

This comment was marked as outdated.

@jayaddison jayaddison changed the title Fixup: reference QUnit.test instead of relying on global namespace entry Fixups: post-merge updates related to PR #6241 Aug 24, 2024
@jayaddison jayaddison marked this pull request as draft August 24, 2024 11:23
@jayaddison
Copy link
Contributor Author

@kevin-brown something about the searching tags does not loose focus integration test seems incompatible with #6241 -- I've commented it out here, and that allows the test suite to pass, but it's not an approach I like so I've moved this into draft status until the problem is figured out.

@jayaddison
Copy link
Contributor Author

The problem seems to be that selection:update event handling in the integration tests isn't isolated per-test-case. Some ideas to resolve this:

  • Add some kind of per-test teardown that removes relevant event handling/selection-update resources (listeners, DOM contents, internal state?).
  • Attach once-only event handlers in each of the relevant integration test cases.
  • Add logic to ensure that each event-handler becomes essentially a no-op despite remaining attached during unrelated integration tests.

I attempted to add a QUnit.reset handler from here to achieve the first; but this did not seem to work.

The second approach doesn't appear straightforward either; there is no .once(...) function available for event-listener attachment on Select2 instances, nor is there a way to pass {once: true} as a parameter to .on(...).

...so I've slightly-reluctantly implemented the third approach. It doesn't genuinely provide any meaningful runtime isolation between the integration test cases, but it ensure that each event listener should only perform any actions within the scope of its parent test case.

@jayaddison jayaddison marked this pull request as ready for review August 24, 2024 14:36
@jayaddison
Copy link
Contributor Author

@kevin-brown it's not ideal code (details in my previous message if you're curious), but I believe this is ready for merge.

@kevin-brown
Copy link
Member

I'll take the "temporary" hack for now. Thanks for looking into this and documenting your findings!

@kevin-brown kevin-brown merged commit 07bd61a into select2:develop Aug 24, 2024
3 checks passed
@jayaddison jayaddison deleted the pr-6241-followup/qunit-access-fixup branch August 24, 2024 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants