fix: start detached and stop cleanup #403
Conversation
Also use the HEALTHCHECK_PATH constant in this helper
The process has handlers to finalize on SIGHUP, but the stop request already finalizes the build before sending SIGHUP. Sometimes this caused duplicate, empty, builds due to the finalize method creating a build when the current build has already been finalized.
Testing with `process.exit` causes the test process to exit
This command was hard to test due to the subprocess being spawn async. In practice the parent process remains open as well, but for our tests this method returned before the command was finished.
These tests previously caused the entire suite to exit early. Once that was fixed, the two tests written with the oclif helper failed. One was updated to not need the helper, and the other test was removed since the snapshot command now has a default directory argument.
This agent test caused many other tests to fail since it mutated the DEFAULT_CONFIGURATION object.
With recent bug fixes, these tests can be unskipped and actually run. The 'echo' test was replaced with 'sleep' to avoid printing to the inherited stdio of the test process. An unfinished test was fleshed out, and the other unfinished test removed since exec does not support that behavior.
When the tests exited early or threw warnings and logs, it went unnoticed in CI since junit reports to a file. Circle only shows tests when they fail, so passed tests or missed tests were never shown.
| @@ -29,7 +29,7 @@ commands: | |||
| name: Unit tests | |||
| command: | | |||
| mkdir -p reports | |||
| $NYC yarn test --reporter mocha-junit-reporter | |||
| $NYC yarn test | |||
There was a problem hiding this comment.
Do we lose the junit output entirely? It's nice to use Circles reporting from the junit reporter
There was a problem hiding this comment.
Unfortunately with mocha, you can technically only have a single reporter. A junit file reporter for Circle failure reporting or being able to see successful CI output using the default reporter?
We could implement a third-party reporter like mocha-multi-reporters, but I thought that could be another PR.
There was a problem hiding this comment.
Yeah that could be another PR. Wanna create a clubhouse / GH issue (whichever)?
|
Maybe we should update the title to reflect better in the changelog? Something that includes fixing the |
|
This fixes more than just |
## [0.19.2](v0.19.1...v0.19.2) (2019-10-28) ### Bug Fixes * start detached and stop cleanup ([#403](#403)) ([2b4662e](2b4662e))
Purpose
An issue was brought up where the start and stop commands did not work as expected with the latest version of agent. In fixing that issue, I discovered that our test suite was not finishing and was exiting with a
0status early.After digging through CircleCI builds, I am unable to determine when these tests started to not be run in CI. It seem the very fist passing build has a warning about Percy exiting. Previous test runs cannot be viewed due to the junit mocha reporter only saving reports to a file.
Approach
Defaulted healthcheck helper argument to the default port and also made use of the healthcheck endpoint constant.
Since flags no longer have defaults, we were passing the string
"undefined"into the detached start process. This was fixed by conditionally setting those flags.The stop endpoint finalized the build, closed the server, and killed the running process. That last part caused builds to be re-finalized, therefore creating empty duplicate builds. This was fixed by cleaning up the PID file rather than outright killing the process. Since the server closes, the process will close anyway.
Use the oclif
this.exitmethod. Usingprocess.exitcaused the mocha process to exit. Oclif exit stops script execution by throwing an error, so callbacks that referenced the stop method had to be adjusted.execneeded to be moved out of the callbacks and called after the subprocess exited. This subprocess was turned into a promise so that errors can be caught and we can always call close without creating an unhandled rejection.All failing tests were fixed and updated. Even the skipped exec tests can be re-enabled. A couple of tests were removed due to them never working or being superseded by another change.
Finally, junit was removed as the sole reporter so we can actually see the results of our tests in CI.