tests: don't save version in snapshot #5
Conversation
shepherdjerred
requested review from
jmwoliver,
a team and
glin
and removed request for
a team
parse_test.go
Outdated
| if value, ok := copiedData[field]; ok { | ||
| values := lo.Map(value, func(item string, _ int) string { | ||
| return re.ReplaceAllString(item, "X") | ||
| }) | ||
| copiedData[field] = values | ||
| } |
There was a problem hiding this comment.
Would it be easier to delete the fields from the map entirely?
| if value, ok := copiedData[field]; ok { | |
| values := lo.Map(value, func(item string, _ int) string { | |
| return re.ReplaceAllString(item, "X") | |
| }) | |
| copiedData[field] = values | |
| } | |
| if value, ok := copiedData[field]; ok { | |
| delete(copiedData, field) | |
| } |
There was a problem hiding this comment.
I guess your intent is to try to preserve the information? I'm not sure if it is useful and I think this could still break if the length of the version string changed from something like 1.2.3 to 1.2.3dev0. Or am I misunderstanding how it replaces?
There was a problem hiding this comment.
I was trying to prevent deleting anything since it reduces how effective these tests are, but you are right that this will cause issues for versions that change length.
There was a problem hiding this comment.
If we wanted to preserve that it is set to something, maybe we can just set it to the field name?
| if value, ok := copiedData[field]; ok { | |
| values := lo.Map(value, func(item string, _ int) string { | |
| return re.ReplaceAllString(item, "X") | |
| }) | |
| copiedData[field] = values | |
| } | |
| if _, ok := copiedData[field]; ok { | |
| copiedData[field] = field | |
| } |
I'm not sure if that is any better than just deleting the fields that change though. I'm good with whichever you think is best!
There was a problem hiding this comment.
Yeah, that's what we did before. I think you're right that it's better.
shepherdjerred
force-pushed
the
sj/filter-version
branch
from
d108705 to
920c18e
Compare
shepherdjerred
force-pushed
the
sj/filter-version
branch
from
920c18e to
c1d3496
Compare
| - cron: "0 0 * * *" | ||
| - cron: "0 0 1 * *" # run on the first of each month |
There was a problem hiding this comment.
This should make test failures due to dependency updates less annoying.
| values := lo.Map(value, func(_ string, _ int) string { | ||
| return fmt.Sprintf("%s exists", field) | ||
| }) | ||
| copiedData[field] = values |
There was a problem hiding this comment.
Should this be simplified back to what it was?
| values := lo.Map(value, func(_ string, _ int) string { | |
| return fmt.Sprintf("%s exists", field) | |
| }) | |
| copiedData[field] = values | |
| copiedData[field] = []string{fmt.Sprintf("%s exists", field)} |
There was a problem hiding this comment.
I don't think so. It's unlikely, but since the input is a list of strings we don't want to assume the length. For example if we somehow send a list of versions or checksums we'd want to know.
As versions update, these tests will start to fail.
@jmwoliver suggested that we could store the built distributions in this repository. I see a little bit of value in using the latest versions of these libraries to ensure that we continue to pull the correct metadata as they update, although that does lead to failures occurring.