🐛 ✅ Fix caching for "Dismiss this Version" in pulsar-updater

[kiskoza]

Identify the Bug

#784

Description of the Change

I found that the code was expecting the local storage key to keep the last-update-check value in cache ifinitely, but it was not provided as an argument, so I added it.

Then I looked into the tests and extended them to cover the fix as well. I added a dummy key to all isItemExpired call to reflect on how the code should work.

While I was adding and fixing theses tests, I found that they produced false positive results because the tests were using fake timers, Date.now() was always 1000 which meant that all cache-tests produced a non-expired cache entry. I set jasmine to use jasmine.useRealClock();

Then, there's the last test: it meant to mock out the online() method which was required from the other file into a cache object. However, this method on the cache is not the same as the one called from the isItemExpired() method from jasmine's perspective, hence the mock wasn't working - see the false positive results. The proper way to check this behaviour would be to mock out window.navigator's onLine property, however, Pulsar is using an old and unmaintained implementation of the jasmine test-framework which doesn't support this feature yet. I wrote how the test should look like and moved it behind a condition, we'll run this only with more recent jasmine version.

Alternate Designs

Possible Drawbacks

Verification Process

I've run the new tests on the package.

Release Notes

• Fixed the issue with the "Dismiss this Version" button

[confused-Techie]

Thanks a ton for contributing!

And this fix looks awesome, unfortunate I didn't see this oversight when originally creating this package, so thanks a ton for finding the root cause!

Otherwise the changes to the tests look good as to fixing the issues present in them, and to covering this change, where the change in actual production code is extremely small. So this is awesome to see!

As for our version of Jasmine, this is something we are aware of and is still part of our baggage from Atom, where it works for the most part, but once you dive deep it becomes problematic, at least for me due to the nearly non-existent documentation.

But beyond all of that, thanks a ton, assuming all tests pass lets get this merged!

[confused-Techie]

LGTM!
Wanted to add approval after some updates, but changes look good. Lets get it merged