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

Fix intermittent test failures #368

Merged

marcotranchino merged 3 commits into use-govuk-frontend-v5 from use-govuk-frontend-v5-fix-intermittent-tests

Oct 8, 2024

Contributor

marcotranchino commented Sep 23, 2024 •

edited

Loading

With this change, we are fixing the intermittent test failures by
updating the tests' setups and afterEach instructions and also by
updating some modules in order to prevent access to properties that
have not yet been initialised properly.

marcotranchino changed the title ~~Fix intermittent tests by cleaning up after each~~ Fix intermittent tests

marcotranchino force-pushed the use-govuk-frontend-v5-fix-intermittent-tests branch from 9b7c06d to 7ec9626 Compare

September 23, 2024 10:40


          Fix intermittent tests by cleaning up after each

982ae30

marcotranchino force-pushed the use-govuk-frontend-v5-fix-intermittent-tests branch from 7ec9626 to 982ae30 Compare

September 23, 2024 10:43

marcotranchino changed the title ~~Fix intermittent tests~~ Fix intermittent test failures


          Prevent access to this.$sticky unless it is ready

7d6a9cc

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at #368

marcotranchino force-pushed the use-govuk-frontend-v5-fix-intermittent-tests branch from 69626bd to 7d6a9cc Compare

September 23, 2024 14:29

marcotranchino mentioned this pull request

Introduce a custom module to silence dart sass warnings #364

Merged

marcotranchino marked this pull request as ready for review

September 23, 2024 14:38

Contributor Author

marcotranchino commented Sep 23, 2024

I have re-run the tests 10 times and they have consistently passed each time.

Contributor

tombye commented Sep 24, 2024

Thanks for doing this @marcotranchino, the tests failing are a real problem for merging changes at the moment.

Forgive me if I'm wrong but reading the changes to lib/assets/javascripts/_modules/table-of-contents.js, I'm not sure they're doing what it looks like. this.$sticky is a jQuery object, the result of a call to the $ function. From what I understand, when called with a CSS selector, as we're doing here, it will always return that object. The only thing that changes is that it will either have DOM nodes, if there are some in the document that match the selector, or not, if there aren't. The normal way to test this is to check the length propery.

So I'm not sure how testing the truthyness of this.$sticky is making these tests pass because it always returns the object and so should always be truthy. A jQuery object is always passed to the StickyOverlapMonitors‎ class when its instantiated and so will always be assigned to this.$sticky. I guess jQuery might not load but that would cause errors higher up the stack, not where we're seeing them.

I might also just be missing something obvious :) What do you think?

Contributor Author

marcotranchino commented Sep 24, 2024 •

edited

Loading

Thank you very much, @tombye !

I am going to have a look at what you said with @iqbalgds and get back to you.

Adding some screenshots of the intermittent errors for reference.


          Update JQuery visibility check on sticky element

96928c9

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
#368 (comment)

[2]
#368

Contributor Author

marcotranchino commented Sep 24, 2024

I have made the suggested change, @tombye, thanks!

Contributor Author

marcotranchino commented Sep 26, 2024

Also this PR can be merged into your branch, @kr8n3r
Let me know if you are happy for me to proceed.

kr8n3r approved these changes

View reviewed changes

marcotranchino merged commit daec63f into use-govuk-frontend-v5

3 checks passed

marcotranchino deleted the use-govuk-frontend-v5-fix-intermittent-tests branch

October 8, 2024 09:47

kr8n3r pushed a commit that referenced this pull request


          Prevent access to this.$sticky unless it is ready

535100b

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at #368

kr8n3r pushed a commit that referenced this pull request


          Update JQuery visibility check on sticky element

a7de263

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
#368 (comment)

[2]
#368

kr8n3r pushed a commit that referenced this pull request


          Prevent access to this.$sticky unless it is ready

9b82eb9

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at #368

kr8n3r pushed a commit that referenced this pull request


          Update JQuery visibility check on sticky element

dc5a72e

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
#368 (comment)

[2]
#368

marcotranchino added a commit that referenced this pull request


          Prevent access to this.$sticky unless it is ready

082858b

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at #368

marcotranchino added a commit that referenced this pull request


          Update JQuery visibility check on sticky element

ee66cb8

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
#368 (comment)

[2]
#368

kr8n3r pushed a commit that referenced this pull request


          Prevent access to this.$sticky unless it is ready

30322df

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at #368

kr8n3r pushed a commit that referenced this pull request


          Update JQuery visibility check on sticky element

eab85ee

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
#368 (comment)

[2]
#368

maverickvi pushed a commit to hmrc/tech-docs-gem that referenced this pull request


          Prevent access to this.$sticky unless it is ready

494bf54

This change adds checks to ensure that this.$sticky exists and it is visible
before trying to access its methods.

Further information at alphagov#368

maverickvi pushed a commit to hmrc/tech-docs-gem that referenced this pull request


          Update JQuery visibility check on sticky element

68e7157

Following a PR review[1], this commit changes the test on this.$sticky, in
order to verify if the object actually contains any elements.

Further details in the PR[2].

[1]
alphagov#368 (comment)

[2]
alphagov#368

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

Labels

None yet