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: use watch strategy #236

Merged
merged 2 commits into from Aug 11, 2019

Conversation

@agraves
Copy link
Contributor

commented Jul 18, 2019

NB: Fixes #232

NB: submitting this for a gut-check / as a proof of concept. There's a lot of style / cleanup work that needs to be done before this is ready for a line-by-line.

Motivation and Context

Let's say you're a new developer working on Polly. You check out the repo, install deps and run $ yarn test. This kicks off the following steps:

  1. yarn clean
  2. yarn build for server & utils
  3. yarn build and test:build for ~12 sundry sub-packages
  4. yarn testem, which kicks off the test runner.

This is a development pain point for the following reasons:

  1. These tasks are run as a pre-hook inside testem, which makes the testing ui unresponsive for the ~2m it take to run the build.
  2. Users are forced to re-run the build process whenever they modify either the tests or the application.
  3. Hitting enter in the testem ui re-triggers the hooks, which can saturate your CPU if you're impatient.

Description

While this required a lot of trial and error, the changes are actually fairly contained:

  1. testem no longer runs any hooks. Running long tasks makes the UI unstable.
  2. testem now only re-runs on changes to build artifacts (code and tests) instead of the source
  3. Every sub-package now has a yarn watch command that triggers a watch on its code and tests
  4. The root package.json now has a yarn watch command that runs all of the sub-packages' watch commands in parallel.

Testing instructions

  • in one tab, run yarn watch and wait for it to settle
  • run yarn test in another tab
  • trigger a failure by, for example, adding "expect(false).to.equal(true)" line 6 of tests/integration/adapter-browser-tests.js
  • observe that polly selectively re-builds the select file and re-triggers the tests

Tradeoffs, deficiencies and future work

General concerns

  1. I decided to go with this approach because it was the only one I could find that didn't require me to take a chainsaw to the actual configs.
  2. The best approach I can find would be to use (rollup/rollup#1389) approach. tl;dr you can pass an array of rollup configs to a single rollup instance. This would allow us to have a single rollup instance watch every sub-package instead of having 15 separate processes, but trying to aggregate the final configs turned out to be really painful (because of liberal use of process.cwd among other things).

Developer workflow

As implemented, this requires developers to manually kick off watch, see when it settles and then start testem. I personally prefer this approach, but I suspect something more streamlined will be required. So, I'd propose the following:

  1. yarn test should run yarn clean && yarn build && yarn testem and then launch yarn watch in the background. I'm not exactly sure how this would be implemented, but quitting testem would have to terminate the watchers.
  2. Developers who want a little more control can use yarn test:watch and yarn build:watch to use the workflow I'm submitting with -- i.e. complete separation of test running and build, which imo is preferable if for example you are booting testem and know that an incremental build is sufficient. (As an aside, an incremental build is actually always sufficient since it can build from scratch -- the problem is that there's no obvious way to block testem until the incremental build has finished, aside from writing something to block that process until every artifact is present).

Merge blockers

  • style check for commit, code changes and so forth
  • test/integration/adapter-node-tests.js seems to not be properly run on master?
  • look into the ember submodule -- seems to be excluded
  • talk to polly team and agree on naming script commands
  • remove redundant script commands

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My code follows the code style of this project.
  • My commits and the title of this PR follow the Conventional Commits Specification.
  • I have read the contributing guidelines.

@agraves agraves force-pushed the agraves:ag-mass-watch branch 2 times, most recently from 3bd35ea to 1a8d7b7 Jul 18, 2019

@@ -2,6 +2,7 @@

export default function adapterNodeTests() {
it('should handle recording requests posting a Buffer', async function() {
expect(false).to.equal(true);

This comment has been minimized.

Copy link
@palmerj3

This comment has been minimized.

Copy link
@agraves

agraves Jul 19, 2019

Author Contributor

This is demonstrating that this test doesn’t appear to be running (even on master) (see merge blocker list)

@jasonmit

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

I believe the quickest win is to disable testem's watcher option by default. I noticed awhile ago that the tests would re-execute midway through a test run and have long suspect it was because of watch_files but never went back to verify.

This morning I found when watch_files is set, the tests execute twice which tells me perhaps that testem is detecting a fs change midway through the test run causing them to re-execute. (I haven't verified if moving to the in memory persister helps avoid this case.)

When disabled, I see a noticeable speed improvement at the the cost of the developer experience in having to hit [enter] to trigger the rebuild - which takes 22s on my machine. I think personally think it's a fine compromise until we determine what folder/files are being watched that shouldn't be. testems on_change hook may give some insights into what is causing it.

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@jasonmit Hey Jason -- appreciate the response & detective work. I agree that in the very short term, looking at the watch behavior will make the development experience much more pleasant.

However, I'm eager to clean up and merge this PR since using a watch strategy means that for repeated test runs (e.g. when you're TDDing) you only re-build the changed files, cutting even that ~22s down to a second or so.

@jasonmit

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@agraves looking forward to seeing this land. Let me know if there's anything I can help with.

test/integration/adapter-node-tests.js seems to not be properly run on master?

Looks like it's currently dead code. I don't see anything importing it so we'll need to look into resolving that. I'd be fine tackling this separately since it's an existing issue.

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@jasonmit ok thanks man! been away at a workshop but will be back on this next week -- excited to get it over the finish line

@agraves agraves force-pushed the agraves:ag-mass-watch branch from 1a8d7b7 to 5c4b9d6 Aug 1, 2019

@agraves agraves force-pushed the agraves:ag-mass-watch branch from 5c4b9d6 to f82c1c1 Aug 1, 2019

@agraves agraves changed the title fix: use watch strategy (NOMERGE) fix: use watch strategy Aug 1, 2019

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Ok guys -- @jasonmit et al -- think this is ready for review.

As mentioned elsewhere, this supports two workflows. There's the existing$ yarn test:ci, which runs the whole suite.

This now adds a second workflow, where you $ yarn test:watch in one tab, wait for it to settle and then $ yarn test in another tab. Modifying a test or package only triggers an incremental build, making the suite highly responsive.

Personally I like the explicit nature of that workflow (and the simplicity of its implementation) but I'm open to adding other workflows as well.

@offirgolan offirgolan requested a review from jasonmit Aug 1, 2019

@jasonmit

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@agraves sounds good, I'll pull it down tomorrow and kick the tires. Thanks again!

@jasonmit

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

The build experience is much better!

Some things I need we need are a helpful error message explaining to run a build (yarn watch) before yarn test. yarn clean && yarn test to see what I mean in terms of the current build time error since unable to import polly deps.

I also think we need to add a section to the README or contributor section explaining this setup before merging.

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Yay! Thanks @jasonmit, glad you liked it. It's a big improvement on our end as well.

I'll go ahead and update the documentation, though I probably won't finish it until early next week.

@jasonmit
Copy link
Member

left a comment

  • README/docs update documenting new test flow
  • Add a message in suggesting to yarn watch if no build exists when executing yarn test

@agraves agraves force-pushed the agraves:ag-mass-watch branch from 795cd4c to 61958b1 Aug 5, 2019

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@jasonmit Ok, pushing out some fixes for this now. Here are the key changes:

  1. Felt it was a little confusing to have yarn test (which runs testem) require you to have the watch process running. So, I renamed yarn test to yarn test:watch and yarn test:ci to yarn test. This feels much clearer to me -- a new user can run yarn test and get the CI red/green, and then when the read the docs, they will learn to use yarn watch and yarn test:watch to run incremental builds. Not super married to this but feel pretty good.
  2. I updated CONTRIBUTING.md, which seems to be the only place that dives into the test commands.
  3. I added a bash script that checks for the existence of a couple key files to detect if you're trying to run yarn test:watch without having run a build. Unfortunately, the standard bash file-exists check doesn't support wildcards, so I just checked for build artifacts generated by core. I could add something that asserts that the wildcards match at least one file but it makes the script a bit more complicated.

@agraves agraves force-pushed the agraves:ag-mass-watch branch from 61958b1 to 5311458 Aug 5, 2019

Aaron Graves
fix: Implement watch strategy for tests & development
* in one tab, run `yarn watch` and wait for it to settle
* run `yarn test` in another tab
* watch should correctly watch all of the packages and tests and
  re-run as needed

@agraves agraves force-pushed the agraves:ag-mass-watch branch from 5311458 to a68f5de Aug 5, 2019

Aaron Graves
fix: Review Fixes
* NB: aliased test:ci to test
* NB: moved testem runner to test:watch
* remove invalid watch spec for rollup files
* remove specialized watch commands since watch covers everything now
* added a script to ensure server, app and tests are built before running testem
* updated persister-in-memory to support new (required) yarn commands

@agraves agraves force-pushed the agraves:ag-mass-watch branch from a68f5de to 0e5e1d9 Aug 5, 2019

@agraves

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Think this is all set from my perspective! 🚀

@jasonmit jasonmit merged commit 5b4edf3 into Netflix:master Aug 11, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Thanks for taking this on @agraves 🥂

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