Update migration sharness tests for new migrations#8053
Conversation
| test_expect_code 1 ipfs daemon > daemon_out 2> daemon_err | ||
| test_expect_code 1 bash -c "echo n | ipfs daemon > daemon_out 2> daemon_err" |
There was a problem hiding this comment.
Why has this part of the test changed? Isn't the prompt for auto-migration code still the same?
There was a problem hiding this comment.
The prompt is the same, but needed to echo n to answer the prompt so that it would not just hang. Needed the bash -c ... to handle the pipe in the command.
There was a problem hiding this comment.
After retesting, I do not know why this change seemed necessary. Changed back to original.
| test_init_ipfs | ||
|
|
||
| # Create fake migration binaries instead of letting ipfs download from network | ||
| # To test downloading and running actual binaries, comment out this test. |
There was a problem hiding this comment.
We weren't testing real binaries here before, right? So this is just a heads up for developers/testers.
Not that it's strictly necessary (as it can cause CI flakiness, slow downs, etc.), but do we happen to already have any integration tests that handle the end-to-end workflow here?
There was a problem hiding this comment.
Correct, before we were also testing a fake migration binary (mocked fs-repo-migrations). No that we do not have one fs-repo-migration, but have separate migration executables, I make fake versions of those now.
The actual migrations are tested in fs-repo-migrations sharness tests. This part tests that the go-ipfs will actually run the migrations.
The part that is missing from a complete end-to-end test is testing go-ipfs downloading migrations from the network.
| grep "Success: fs-repo migrated to version 11" true_out > /dev/null | ||
| ' | ||
|
|
||
| # Daemon exits with 1 after the prompt gets "n" when asking to migrate. |
There was a problem hiding this comment.
It's not getting "n" anymore, just waiting right?
There was a problem hiding this comment.
yes. attention to detail, thank you. Test description is sufficient, no comment needed.
| # If run with real migrations, the daemon continues running and must be killed. | ||
| test_expect_success "ipfs daemon --migrate=true runs migration" ' | ||
| test_expect_code 1 ipfs daemon --migrate=true > true_out | ||
| test_expect_code 1 ipfs daemon --migrate=true > true_out 2>&1 |
There was a problem hiding this comment.
What is this change doing for us?
There was a problem hiding this comment.
That was needed because PR #8054 was not merged yet. Some of the output was now going to stderr that was expected to go to stdout. That made a subsequent test, that scanned true_out fail, but for the wrong reasons.
There was a problem hiding this comment.
If I rebase on top of #8054, then that change can go away.
There was a problem hiding this comment.
Sounds good, let's rebase now that the bug is fixed and keep it as is. Then it looks like we're ready to merge 😄
There was a problem hiding this comment.
Done. All tests passing.
With the new migrations, go-ipfs no longer uses fs-repo-migrations to do repo migrations, and was downloading real migration binaries from the network and running them. This caused failure, but was not caught because the test was expecting `ipfs daemon --migrate` to fail for other reasons. This PR fixes the migration tests by creating the appropriate fake migration binaries in the PATH so that those get run and avoid downloading the real ones. This also fixes a test that was previously marked broken.
gammazero
force-pushed
the
fix/migration-sharness-tests
branch
from
4f9e574 to
66db9a1
Compare
2 participants
With the new migrations, go-ipfs no longer uses
fs-repo-migrationsto do repo migrations, and was downloading real migration binaries from the network and running them. This caused failure, but was not caught because the test was expectingipfs daemon --migrateto fail for other reasons.This PR fixes the migration tests by creating the appropriate fake migration binaries in the
PATHso that those are run instead of downloading the real ones. This also fixes a test that was previously marked broken.