upgrade from more recent, 2.7 and 2.8 #9807
Conversation
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9807 +/- ##
==========================================
- Coverage 59.31% 58.85% -0.46%
==========================================
Files 586 586
Lines 70673 70673
==========================================
- Hits 41917 41595 -322
- Misses 28171 28545 +374
+ Partials 585 533 -52 ☔ View full report in Codecov by Sentry. |
| return errors.New("not ready") | ||
| } | ||
| return nil | ||
| }) | ||
| }, | ||
| func(t *testing.T, ctx context.Context, c *client.APIClient, _ string) { /* postUpgrade */ |
There was a problem hiding this comment.
git doesn't do a good job with this diff because of an indentation change from this line in my new changes
if semver.Compare("v"+from, "v2.8.0") < 0 {
There was a problem hiding this comment.
I'm wary about changing the magic numbers. We should figure out how to rewrite the test so that they don't exist. Maybe it's a bug in the upgrade. cc: @acohen4 @brycemcanally
| require.NoError(t, err) | ||
| expectedCommitCount := getExpectedCommitCountFromVersion(from) |
There was a problem hiding this comment.
I feel like pre-upgrade, this number should be stable. It should always be 11 or 12. I wonder what listCommits is returning.
There was a problem hiding this comment.
let me add a log loop. let's see what happens
There was a problem hiding this comment.
There was a problem hiding this comment.
src/testing/deploy/upgrade_test.go
Outdated
| logCommits := func(t *testing.T, c *client.APIClient, commits []*pfs.CommitInfo) error { | ||
| var buf bytes.Buffer | ||
| for i, commit := range commits { | ||
| c.GetFile(commit.Commit, "/hello", &buf) |
There was a problem hiding this comment.
You should check the error on this.
| err := c.GetFile(commit.Commit, "/hello", &buf) | ||
| commitFile := buf.String() | ||
| buf.Reset() | ||
| if err != nil || commitFile == "" { |
There was a problem hiding this comment.
I'd check err before accessing anything on buf. If there's an error you can't really expect anything of the output. It could be a nil pointer here
There was a problem hiding this comment.
buf is always ready to use regardless of the outcome of GetFile, but it will obviously be annoying when the output says the file is empty, when in reality there was some error.
In this PR, I'm changing the
fromVersionsto be more recent so we can emulate customer upgrade paths better. Unfortunately, changing those upgrade paths also meant a lot of other changes to theupgradeTriggertest expectations and logic. I added some comments and moved some magic numbers to the top.This issue was very difficult because pachyderm handles trigger commits differently after 2.7.x. We think verifying the file contents of each commit directly would be the best approach. That solution would require more time, and i've already got log messages with the file contents, so we think these changes are good enough for now.
In addition, I tried updating from
2.9.2and got a new error.because I wasn't able to correctly verify 2.9.2, we want to reassign this test case to the PFS team to take a look and hopefully use
waitCommitand file contents in their verification process. Here's the new issue.