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

Only run tests for modules affected by a PR #1579

Closed
stephenplusplus opened this issue Sep 8, 2016 · 10 comments
Closed

Only run tests for modules affected by a PR #1579

stephenplusplus opened this issue Sep 8, 2016 · 10 comments
Assignees

Comments

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Sep 8, 2016

Cont'd from #1576 (comment)

We now have 3 OSes to run tests for 3 different versions of Node. Running the system tests takes a big portion of the time, the other is the installation of all of our dependencies. I have improved the performance somewhat by forcing Travis to use npm 3, which flattens the dependency tree before downloading.

I think there might be room to improve if we only run the system tests for the module that the modified files affected, e.g., a merge that only changed Storage files wouldn't run Bigtable tests.

@callmehiphop any ideas on tooling that might exist for this? I know we've indirectly used things like this for Grunt/Gulp pipelines.

@swcloud
Copy link
Contributor

swcloud commented Sep 8, 2016

Cool, I like this proposal.

@callmehiphop
Copy link
Contributor

callmehiphop commented Sep 8, 2016

Not sure if Grunt or Gulp would really help us here, usually they use some kind of local caching mechanism to figure out which files have changed and which to test against.

We could probably solve this with a shell script? Not sure if this is gross or not, but we can get a list of files that have changed pretty easily

$ git remote add googs git@github.com:GoogleCloudPlatform/google-cloud-node.git
$ git fetch googs
$ git diff --name-only HEAD googs/master

At which point we could probably match the changed file to an exact unit test file (I believe we're 1:1 on those?)

This would also allow us to conditionally run system tests on PRs, but I'm not sure how I feel about that - especially since this issue was opened directly in response to Travis jobs taking forever to run.

@stephenplusplus
Copy link
Contributor Author

but I'm not sure how I feel about that

Why the uncertainty?

@callmehiphop
Copy link
Contributor

They have the tendency to run a lot slower than unit tests, not to mention for common package changes we would probably want to test against everything, so those kind of PRs would take even longer.

@stephenplusplus
Copy link
Contributor Author

This would also allow us to conditionally run system tests on PRs,

Oh, it just clicked that I was talking about running system tests on PRs. We definitely don't want to do that, sorry. We should conditionally run sections of the unit tests on merges, not PRs. I'll edit my post.

For PRs, can you think of anything we can do to speed them up?

@callmehiphop
Copy link
Contributor

callmehiphop commented Sep 13, 2016

So, I've been playing with this and here is my naive attempt - I'm not the greatest at Bash 😢

# we'll use this remote to do our diff
git remote add googs https://github.com/GoogleCloudPlatform/google-cloud-node.git
git fetch googs

# create a unique array of modules that have been updated
MODULES=$(git diff HEAD googs/master --name-only --exit-code | grep "packages" | cut -d \/ -f 2 | sort -u)

function get_test_files {
  for i in ${MODULES}; do
    echo "packages/$i/$1/*.js"
  done
}

if [ -n "$MODULES" ]; then
  # unit tests
  mocha --timeout 5000 --bail $(get_test_files "test")

  # system tests
  mocha --no-timeouts --bail $(get_test_files "system-test")
fi

I think the most glaring issue with this is that modules that have dependencies on other modules are obviously not being tested, but we could easily expand on this to add some rules..?

WDYT? I can make a PR if it looks ok to you guys.

@stephenplusplus
Copy link
Contributor Author

PR it up 👍

@stephenplusplus
Copy link
Contributor Author

I think the most glaring issue with this is that modules that have dependencies on other modules are obviously not being tested, but we could easily expand on this to add some rules..?

Just an idea, we could read the package's package.json to see if it depends on the affected module and add it to the mocha test pattern.

@callmehiphop
Copy link
Contributor

Just an idea, we could read the package's package.json to see if it depends on the affected module and add it to the mocha test pattern.

Sure could! What's the scope of this? PR only? Or merges as well ..?

@stephenplusplus
Copy link
Contributor Author

Both, both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants