fix: use watch strategy #236

agraves · 2019-07-18T20:15:06Z

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:

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

This is a development pain point for the following reasons:

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.
Users are forced to re-run the build process whenever they modify either the tests or the application.
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:

testem no longer runs any hooks. Running long tasks makes the UI unstable.
testem now only re-runs on changes to build artifacts (code and tests) instead of the source
Every sub-package now has a yarn watch command that triggers a watch on its code and tests
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

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.
The best approach I can find would be to use (Support multiple configs/bundles in parallel 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:

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.
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.

palmerj3 · 2019-07-18T23:59:24Z

tests/integration/adapter-node-tests.js

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

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


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

jasonmit · 2019-07-21T18:58:03Z

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 · 2019-07-22T16:41:02Z

@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 · 2019-07-23T06:47:07Z

@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 · 2019-07-26T15:13:22Z

@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 · 2019-08-01T18:41:31Z

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.

jasonmit · 2019-08-02T04:05:42Z

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

jasonmit · 2019-08-02T18:59:22Z

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 · 2019-08-02T19:15:06Z

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

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

agraves · 2019-08-05T18:52:14Z

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

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.
I updated CONTRIBUTING.md, which seems to be the only place that dives into the test commands.
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.

* 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

* 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 · 2019-08-05T20:49:22Z

Think this is all set from my perspective! 🚀

jasonmit · 2019-08-11T21:40:29Z

Thanks for taking this on @agraves 🥂

agraves force-pushed the ag-mass-watch branch 2 times, most recently from 3bd35ea to 1a8d7b7 Compare July 18, 2019 20:44

palmerj3 reviewed Jul 18, 2019

View reviewed changes

agraves force-pushed the ag-mass-watch branch from 1a8d7b7 to 5c4b9d6 Compare August 1, 2019 16:08

agraves mentioned this pull request Aug 1, 2019

some integration tests are not being run #243

Closed

agraves force-pushed the ag-mass-watch branch from 5c4b9d6 to f82c1c1 Compare August 1, 2019 16:17

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

offirgolan requested a review from jasonmit August 1, 2019 19:55

offirgolan added the build 🚧 label Aug 1, 2019

jasonmit suggested changes Aug 3, 2019

View reviewed changes

agraves force-pushed the ag-mass-watch branch from 795cd4c to 61958b1 Compare August 5, 2019 18:44

agraves force-pushed the ag-mass-watch branch from 61958b1 to 5311458 Compare August 5, 2019 18:54

fix: Implement watch strategy for tests & development

855ffe1

* 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 force-pushed the ag-mass-watch branch from 5311458 to a68f5de Compare August 5, 2019 19:42

agraves force-pushed the ag-mass-watch branch from a68f5de to 0e5e1d9 Compare August 5, 2019 19:49

offirgolan approved these changes Aug 11, 2019

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use watch strategy #236

fix: use watch strategy #236

agraves commented Jul 18, 2019 •

edited

Loading

palmerj3 Jul 18, 2019

agraves Jul 19, 2019

jasonmit commented Jul 21, 2019 •

edited

Loading

agraves commented Jul 22, 2019

jasonmit commented Jul 23, 2019 •

edited

Loading

agraves commented Jul 26, 2019

agraves commented Aug 1, 2019

jasonmit commented Aug 2, 2019

jasonmit commented Aug 2, 2019 •

edited

Loading

agraves commented Aug 2, 2019

jasonmit left a comment •

edited

Loading

agraves commented Aug 5, 2019

agraves commented Aug 5, 2019

jasonmit commented Aug 11, 2019

fix: use watch strategy #236

fix: use watch strategy #236

Conversation

agraves commented Jul 18, 2019 • edited Loading

Motivation and Context

Description

Testing instructions

Tradeoffs, deficiencies and future work

General concerns

Developer workflow

Merge blockers

Types of Changes

Checklist

palmerj3 Jul 18, 2019

Choose a reason for hiding this comment

agraves Jul 19, 2019

Choose a reason for hiding this comment

jasonmit commented Jul 21, 2019 • edited Loading

agraves commented Jul 22, 2019

jasonmit commented Jul 23, 2019 • edited Loading

agraves commented Jul 26, 2019

agraves commented Aug 1, 2019

jasonmit commented Aug 2, 2019

jasonmit commented Aug 2, 2019 • edited Loading

agraves commented Aug 2, 2019

jasonmit left a comment • edited Loading

Choose a reason for hiding this comment

agraves commented Aug 5, 2019

agraves commented Aug 5, 2019

jasonmit commented Aug 11, 2019

agraves commented Jul 18, 2019 •

edited

Loading

jasonmit commented Jul 21, 2019 •

edited

Loading

jasonmit commented Jul 23, 2019 •

edited

Loading

jasonmit commented Aug 2, 2019 •

edited

Loading

jasonmit left a comment •

edited

Loading