Fixes #1047: preventDefault()

[sadiqkhoja]

Closes #1047

Builds are failing on the master branch as well, this fix is necessary now.

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?

This solves the problem, but I am open to hearing other options. @matthew-white mentioned something about preventDefault in tests in the issue, I don't where to set that.

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?

No

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

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

[matthew-white]

This test failure is a weird one! I've spent some time looking into it, and I can't quite figure it out.

I actually don't think that the test failure has to do with the fact that the test causes a file to be downloaded. The first commit in this PR called preventDefault(), which prevents the file download and is a strategy we take in other tests. However, that commit didn't fix the test failure. The error message for the test failure also doesn't mention a problem with a file download or page reload or anything like that:

TypeError: Cannot read properties of undefined (reading 'should')

It looks like it's this line that's failing:

a.attributes().download.should.equal('trees 20241231012345.csv');

In other words, the download attribute isn't being set (it's undefined). When I add logging, I see that the setFilename() function in the component isn't being called. If I add another @click event handler to the root element of the component, I see that that event handler also isn't called. Yet if I use Vue Test Utils to add an event listener (component.element.addEventListener()), I see that that listener is called. Even though a.trigger('click') is called, it's as if Vue event handlers aren't firing.

Another strange thing I noticed is that if I remove the Sinon timer, the setFilename() function is called:

-const clock = sinon.useFakeTimers(Date.parse('2024-12-31T01:23:45'));

The test still fails, because the download attribute isn't set to the expected timestamp, but the attribute is at least set to something.

There also seems to be something going on with test order:

• If the failing test is run on its own, it passes.
• If only the test's describe() is run (totaling 2 tests), that fails.
• If I duplicate the test (i.e., if I run it twice), then I see that the first run fails, but the second run succeeds.
• If I reorder the tests in the describe(), running the failing test before the other test, then I see that both tests succeed. You can see that at Reordering tests causes them to pass #1126.

I really don't see anything weird about the other test in the describe() or in the component itself that might explain these issues. It's not really a complicated component! I've spent a bit of time looking into this, and while I'd like to get to the bottom of it, I think it's probably time to move on. @sadiqkhoja, let me know what you think and if you have any other ideas that you think we should explore.

Otherwise, I think it makes sense to merge this PR. This PR calls setFilename() directly. That's something that we generally avoid doing, because it ties the test to the specific implementation of the component. Triggering a click would match what a user would do, but unfortunately, that just isn't working.

[sadiqkhoja]

I ran only two tests in data-template.spec.js and saw this error "Some of your tests did a full page reload!". Searching for that error I landed on karma-runner/karma#3887

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, see #1129.

Then I thought it has to do something with async so I replaced async/await in the second test to promise chain, and it didn't work, see #1128.

As you noted, tests works without Sinon as well, only that we can't assert the timestamp in the filename.

Maybe latest versions of Chrome are reloading the page only if async is used, which is triggering this code in Karma. Or maybe Sinon is doing something that is cause Chrome to misbehave. I don't know who is the culprit here Sinon, Karma or Chrome 🤷‍♂️

I think #11289 is a hack, we can go with that instead of calling setFilename() from the test.

[matthew-white]

I don't know who is the culprit here Sinon, Karma or Chrome 🤷‍♂️

I know, it's all very perplexing! I like how #1129 looks, and I just approved that. But this PR also looks good if you'd prefer to merge this one. Your call! Thank you for fixing this test. 🙏

[matthew-white]

Closing in favor of #1129.