Skip to content

Conversation

@robertbrignull
Copy link
Contributor

Makes all integration tests use the latest bundle version by default. There is one test that already runs on latest and the version in the environment, to test that code path as well.

By running on the latest version of the tools we'll get better confidence when we upgrade the bundle. Currently the CI checks when updating the bundle are basically useless because it uses the tools from the environment.

I also run into this with #222 but I realise that PR is held up by the virtual environment being updated anyway. So if anything using the latest version in our tests would have actually masked the error on that PR and made it more dangerous. It's dangerous if our testing environment is different from the environment that users see.

I guess the ultimate option would be to run all tests on both tools versions, but that is probably prohibitively expensive given the number of checks we already have.

So there are pros and cons of making this change. What do people think?

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@cbraynor
Copy link
Contributor

cbraynor commented Nov 6, 2020

The original suggestion was to run them all on both, but as you mention this takes too long and is too expensive.

It seems we have to choose our blind spot - do we choose to have confidence that code changes here work in the environment that most users will be using (the bundled version) or do we choose to have confidence while upgrading a CodeQL version?

The original consensus was for confidence in ongoing changes, because bundle upgrades are less frequent and can be tested manually. But if others want to revisit this or we want to hedge our bets with some on latest and some on bundled then that's not a particularly strong argument for not doing this.

@robertbrignull
Copy link
Contributor Author

The original consensus was for confidence in ongoing changes, because bundle upgrades are less frequent and can be tested manually.

I think that a fairly compelling argument for keeping things as they are and having these tests use the bundled version.

Upon reflection, I will close this PR. I'll make sure there's a note in the bundle upgrade instructions noting that manual testing is required and that the automated tests won't automatically pick up the new bundle.

@robertbrignull robertbrignull deleted the latest_tools_tests branch April 23, 2021 12:12
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

Successfully merging this pull request may close these issues.

3 participants