Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for "pinned" tests #2803

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Dec 5, 2023

Whenever we bump the RPM version in cmake, certain tests also need updating, e.g. those that compare package digests or RPM sonames as those are subject to the version number.

Up until now, this had to be done manually by running the test-suite, looking at the failed tests' diffs and fixing up the expected outputs by hand.

No more! Separate those tests and add the "make pinned" cmake target to update them automatically. This makes use of the new-ish atshell feature which runs a shell/command (*.sh scripts in tests/pinned/ in this case) in the standard test environment in order to get the new outputs.

See the included README for more details.

@dmnks dmnks added the test Testsuite-related label Dec 5, 2023
@dmnks dmnks force-pushed the static-tests branch 2 times, most recently from 9234823 to 2973e19 Compare December 5, 2023 17:00
@pmatilai
Copy link
Member

pmatilai commented Dec 8, 2023

Heh, I was so used to the old procedure that the idea of somehow improving it never crossed my mind 😅

"static" as a target name is problematic though, "static" in the build system context usually refers to static linkage. Maybe "pinned", or as per the the README, or "check-pinned" to make it clear it relates to tests?

@dmnks
Copy link
Contributor Author

dmnks commented Dec 8, 2023

Yup, I knew the "static" name probably wasn't great because of what you said but also because those tests aren't really "static" any more than the normal tests, actually quite the opposite 😄

For some reason, "pinned" doesn't sound very descriptive either, though. And "check-pinned" even less so... Also, in that case we should probably move all the test-related targets to the "check-" prefix (which gets unwieldy pretty quickly 😄 ).

But "static" is a no-go, I agree. I'll give it a quick thought and do a fixup. Thanks!

@pmatilai
Copy link
Member

pmatilai commented Dec 8, 2023

Pinned is not really descriptive but at least it wont be confused with something else in the build system domain. But yup, perhaps we can come up with something actually descriptive. Names are hard 😆

@pmatilai
Copy link
Member

pmatilai commented Dec 8, 2023

For other random ideas, perhaps something better would be in the direction of what it actually does, eg "version-diff", or what it relates to: preparing for release (release-prep) or such.

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

"version-diff" indeed is perhaps closer to what it does, however it just happens that these tests depend on the version. There might as well be other tests (in the future) that depend on some other baked-in value.

"release-prep" is similarly too generic because this really is just about the tests, not necessarily about an imminent release (e.g. we need to "pin" these tests even if we bump the micro version to .99 on master). But yup, I was too thinking of having a "release" target that would automate some of the release prep work, in which case this target would be made a dependency of it.

Other ideas:

  • rebase-tests
  • reproducible-tests (as this is the topic of the signature-verification test)

Edit: "reproducible-tests" would also be a bit too specific (to that one test) but the general idea perhaps is along the same lines for all such tests... not sure.

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

The idea behind this kind of tests is basically this: We need to track the expected output in the repository so that we ensure it's always the same when running the given test, which is why we don't just dynamically determine the expected output at test-runtime. At the same time, we need this output to be easily regenerated based on the current repo content.

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

... which makes me think about another target name: update-tests 😄

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

And perhaps, if we were to put all the test-related targets behind the "check-" prefix, then "check-update"... (apt-get anyone? 😄 )

@pmatilai
Copy link
Member

pmatilai commented Dec 11, 2023

Leafing through the mental dictionary of handy words...

Mutable! It fits the bill pretty perfectly as these results are expected to change at certain times, whereas all the other tests do not. So perhaps "update-mutable"? You said it yourself: these are not static but quite the opposite.

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

Oh! That's a pretty good idea! 😄

... except that "mutable" is already used in the test context. But - that doesn't matter much, I suppose. It's a lower-level detail that's relevant when writing a test. Otherwise, "mutable" is indeed the best fit.

So, let's just go with that.

@pmatilai
Copy link
Member

pmatilai commented Dec 11, 2023

Yeah, mutable and immutable are generic concepts and used in multiple places already. So if you really want to pinpoint it, "update-mutable-tests" 😄 (but of course that's already annoyingly long)

@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

Yup, annoyingly as well as amusingly long 😄 But as a rule of thumb, naming make targets after the "artifacts" they produce rather than the "action" they do is always better, at least IMO (of course we don't follow that precisely but that's fine).

Whenever we bump the RPM version in cmake, certain tests also need
updating, e.g. those that compare package digests or RPM sonames as
those are subject to the version number.

Up until now, this had to be done manually by running the test-suite,
looking at the failed tests' diffs and fixing up the expected outputs by
hand.

No more!  Separate those tests and add the "make pinned" cmake target to
update them automatically.  This makes use of the new-ish atshell
feature which runs a shell/command (*.sh scripts in tests/pinned/ in
this case) in the standard test environment in order to get the new
outputs.

See the included README for more details.
@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

OK, in the end I went with simply pinned. When I was updating the commit, it became apparent that "mutable" just didn't fit well in the context, and also clashed with the other meaning I'd mentioned...

@dmnks dmnks changed the title Extract static tests for easier release bumps Add support for "pinned" tests Dec 11, 2023
@dmnks
Copy link
Contributor Author

dmnks commented Dec 11, 2023

I also updated the README to make it a bit more clear.

@pmatilai
Copy link
Member

pmatilai commented Jan 4, 2024

Three weeks should be enough think-time for this and since I didn't come up with anything...

@pmatilai pmatilai merged commit 647af12 into rpm-software-management:master Jan 4, 2024
1 check passed
@dmnks
Copy link
Contributor Author

dmnks commented Jan 4, 2024

Thanks 😄 It's basically a private target only used by a handful of people so we have some leeway in terms of choosing a better name and/or implementation of this in the future, but for now, this should suffice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Testsuite-related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants