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

Add preventDefault + hack to avoid karma throwing some of your tests … #1129

Merged

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented Jan 24, 2025

Closes #1047 and makes all test pass which are currently failing.

What has been done to verify that this works as intended?

All tests are passing and no file is created on the file system.

Why is this the best possible solution? Were any other approaches considered?

preventDefault fixes #1047.

Fix for failing test:

This is mysterious fix, I don't exactly know the root cause. The issue seems related to karma-runner/karma#3887 because when I ran only two tests in data-template.spec.ts, I saw this error "Some of your tests did a full page reload!".

One suggestion over there is to add following:

beforeEach(async () => {
    window.onbeforeunload = () => "Oh no!";
});

So I added that and it worked. It didn't make sense to me so I removed the onbeforeunload, just kept beforeEach(async () => {}; and it worked as well. That's what this PR contains.

Alternative solution: #1122

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

None

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@sadiqkhoja sadiqkhoja mentioned this pull request Jan 25, 2025
2 tasks
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Very strange that this works, but I'm glad that it does! I like how this looks. 🎉 Thank you for the fix and for looking into this!

@sadiqkhoja sadiqkhoja marked this pull request as ready for review January 28, 2025 23:31
@sadiqkhoja sadiqkhoja merged commit c77b9bf into getodk:master Jan 28, 2025
1 check passed
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.

Tests create a new "trees 20241231012345.csv" file in "~/Downloads"
2 participants