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

fix snapshotting #93

Merged
merged 3 commits into from Jul 2, 2018

Conversation

Projects
None yet
1 participant
@annthurium
Copy link
Contributor

annthurium commented Jul 2, 2018

When I tried to bump the metrics version in atom/atom to 1.4.0, snapshotting tests failed with the following error:

ReferenceError: atom is not defined
    at Object.../node_modules/metrics/lib/store.js (/Users/distiller/atom/out/startup.js:55678:54)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/metrics/lib/reporter.js (/Users/distiller/atom/out/startup.js:55351:21)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.../node_modules/metrics/lib/metrics.js (/Users/distiller/atom/out/startup.js:15056:24)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at Object.<anonymous> (/Users/distiller/atom/out/startup.js:158:11)
    at Object.../src/initialize-application-window.js (/Users/distiller/atom/out/startup.js:235:10)
    at customRequire (/Users/distiller/atom/out/startup.js:94:47)
    at generateSnapshot (/Users/distiller/atom/out/startup.js:351619:3)
Error: Command failed: /Users/distiller/atom/out/Atom.app/Contents/MacOS/Atom /Users/distiller/atom/script/verify-snapshot-script /Users/distiller/atom/out/startup.js
/Users/distiller/atom/out/startup.js:55678
      const store = new telemetry.StatsStore('atom', atom.getVersion(), atom.inDevMode())

In order to fix this, I need to defer creating the StatsStore until atom has been defined. The approach I took here was to have a wrapper function that memoizes instantiating StatsStore. This function isn't called until we try to do something to the store, at which time atom will exist.

This approach requires less fiddling to make the tests work than some of the other approaches I considered (such as making store a property of the Reporter class, which would have required an amount of spying on Reporter.prototype.someMethod).

...alternately, I could have blocked the files from the metrics package from being included in snapshot tests but that felt hacky to me.

Test plan:

  • run all the unit tests
  • Manually call Reporter.getStore().reportStats() and verify that stats are sent to Central as they should be.

I wish there was a better way to verify that this fixes the snapshot issue before merging...

bitmoji

@annthurium annthurium merged commit 48c8f6b into master Jul 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-jun-fix-snapshotting branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.