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 errors causing tests to fail when run from the UI #17750

Merged
merged 3 commits into from Aug 10, 2018

Conversation

Projects
None yet
2 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Jul 26, 2018

This fixes some errors and noise in the console that has been occurring for me when running the tests in the UI.

  1. There was a test in atom-environment-spec.js that emitted an auto-update-related event in the main process. This caused exceptions to be thrown in all of your other Atom windows. I found that this code path is now covered by other tests, so I simply removed the offending test.
  2. Our strategy for subscribing to multiple different ipc messages in application-delegate.js entailed adding many individual ipc listeners, resulting in frequent EventEmitter leak detected warnings. I've refactored this code to use a single event listener.

maxbrunsfeld added some commits Jul 26, 2018

Delete redundant test for on atom.onUpdateAvailable
This test emitted an event in the main process, which caused exceptions 
in all *other* Atom windows 🙀.
@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Jul 26, 2018

Member

Quick question about quick-resetting project paths: is there any project state that might not be cleared out if calling reset() with the existing paths results in basically a no-op?

Member

daviwil commented Jul 26, 2018

Quick question about quick-resetting project paths: is there any project state that might not be cleared out if calling reset() with the existing paths results in basically a no-op?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 10, 2018

Contributor

I decided to remove my changes regarding the project paths. This was an attempt to fix a problem that had a deeper cause: a performance regression in libgit2 made instantiating GitRepository objects much more expensive. I have now downgraded git-utils to avoid that performance regression, so it's no longer necessary to avoid this code path so carefully.

Contributor

maxbrunsfeld commented Aug 10, 2018

I decided to remove my changes regarding the project paths. This was an attempt to fix a problem that had a deeper cause: a performance regression in libgit2 made instantiating GitRepository objects much more expensive. I have now downgraded git-utils to avoid that performance regression, so it's no longer necessary to avoid this code path so carefully.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 10, 2018

Contributor

The Circle CI failure is due to us no longer using Circle CI, and the VSTS windows failure is an unrelated flaky test.

Contributor

maxbrunsfeld commented Aug 10, 2018

The Circle CI failure is due to us no longer using Circle CI, and the VSTS windows failure is an unrelated flaky test.

@maxbrunsfeld maxbrunsfeld merged commit 8778498 into master Aug 10, 2018

2 of 4 checks passed

ci/circleci Your CircleCI tests were canceled
Details
VSTS: Atom Pull Requests 20180810.5 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-reduce-test-noise branch Aug 10, 2018

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