Skip to content

Conversation

smoser
Copy link
Contributor

@smoser smoser commented Apr 28, 2023

Commit 9671d8f disabled the caching.bats test "test can read previous version's cache".

This re-enables that test after updating it to use a specific value (v1.0.0-rc4) as the "previous version".

The change to use a specific version and use its released binary improves the test. That improves the test for the following reasons:

  • This should be faster - we don't have to build stacker twice.
  • Ultimately testing against the last released version is probably what you want... that is the cache that your users have, not the last development commit.
  • Building stacker from another version is both slow and hard to guarantee. This removes the assumption that the current build environment can build another version of stacker.
  • There was assumed knowledge about both the current and other stacker build systems in the 'make LXC_BRANCH=$(grep ...)' command.
  • The prior version of the test used "current/main" as "previous", but current/main is a moving target. Re-running a checkout at a future point in time may fail as current/main has moved forward. (Think about using 'git bisect' to diagnose a past regression)
  • Even when working as intended, a series of PRs could still break re-use of the cache. Given PRs A, B, C that all are merged to master. B is tested against A, C is tested against B, but nothing tested C against A.

The change here does mean the following things are true, which are possibly undesirable:

  • There is a requirement to update the old version string. v1.0.0-rc4 is an arbitrary value.
  • The old binary must run in the current build environment. This may be hard to guarantee. I'm honestly not sure if static binary will be more reliable than building and executing a specific old version of source.

@smoser smoser requested review from rchincha and hallyn as code owners April 28, 2023 13:57
@smoser smoser force-pushed the fix/re-enable-test branch from 9ac6b54 to d07322f Compare April 28, 2023 13:58
@smoser
Copy link
Contributor Author

smoser commented Apr 28, 2023

#461 disabled the test because it would fail.

@smoser smoser force-pushed the fix/re-enable-test branch 2 times, most recently from c2db32f to 71afb85 Compare April 28, 2023 15:13
# some additional testing that the cache can be read by older versions of
# stacker (cache_test.go has the full test for the type, this just checks
# the mechanics of filepaths and such)
local relurl="https://github.com/project-stacker/stacker/releases/download"
local oldver="v1.0.0-rc4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oldver=$(git describe --abbrev=0 --tags)

Commit 9671d8f disabled the caching.bats test
"test can read previous version's cache".

This re-enables that test after updating it to use a specific value
(v1.0.0-rc4) as the "previous version".

The change to use a specific version and use its released binary
improves the test. That improves the test for the following
reasons:

 * This should be faster - we don't have to build stacker twice.
 * Ultimately testing against the last released version is probably what
   you want... that is the cache that your users have, not the last
   development commit.
 * Building stacker from another version is both slow and hard to
   guarantee. This removes the assumption that the current build
   environment can build another version of stacker.
 * There was assumed knowledge about both the current and other stacker
   build systems in the 'make LXC_BRANCH=$(grep ...)' command.
 * The prior version of the test used "current/main" as "previous", but
   current/main is a moving target.  Re-running a checkout
   at a future point in time may fail as current/main has moved forward.
   (Think about using 'git bisect' to diagnose a past regression)
 * Even when working as intended, a series of PRs could still break
   re-use of the cache.  Given PRs A, B, C that all are merged to
   master.  B is tested against A, C is tested against B, but
   nothing tested C against A.

The change here *does* mean the following things are true, which are
possibly undesirable:

 * There is a requirement to update the old version string.
   v1.0.0-rc4 is an arbitrary value.
 * The old binary must run in the current build environment.  This
   may be hard to guarantee.  I'm honestly not sure if static binary
   will be more reliable than building and executing a specific old
   version of source.

Signed-off-by: Scott Moser <smoser@brickies.net>
@smoser smoser force-pushed the fix/re-enable-test branch from 71afb85 to bc5570d Compare April 28, 2023 17:13
@smoser smoser merged commit b4f7294 into project-stacker:main Apr 28, 2023
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