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

action, conftest: initial xfail support #95

Merged
merged 6 commits into from
Aug 1, 2023
Merged

action, conftest: initial xfail support #95

merged 6 commits into from
Aug 1, 2023

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Aug 1, 2023

This adds an xfail input that configures a whitespace-separated list of test names to mark as xfail. These xfail tests are "strict", meaning that an unexpected pass with them causes a test failure, rather than a silent pass.

Some thoughts:

  • Maybe it makes sense to support fnmatch syntax here, e.g. test_signature_verify::*?

Closes #87.

Closes #94.

@woodruffw
Copy link
Member Author

Example of strict xfail failing (since the tests actually pass):

============================= test session starts ==============================
platform linux -- Python 3.11.4, pytest-7.1.3, pluggy-1.2.0
rootdir: /home/runner/work/sigstore-conformance/sigstore-conformance
collected 15 items

test/test_bundle.py .......                                              [ 46%]
test/test_certificate_verify.py .                                        [ 53%]
test/test_signature_verify.py FF...                                      [ 86%]
test/test_simple.py ..                                                   [100%]

=================================== FAILURES ===================================
______________________ test_verify_empty[BundleMaterials] ______________________
[XPASS(strict)] skipped by suite runner
_______________ test_verify_empty[SignatureCertificateMaterials] _______________
[XPASS(strict)] skipped by suite runner
=========================== short test summary info ============================
FAILED test/test_signature_verify.py::test_verify_empty[BundleMaterials]
FAILED test/test_signature_verify.py::test_verify_empty[SignatureCertificateMaterials]
=================== 2 failed, 13 passed in 70.70s (0:01:10) ====================

@woodruffw woodruffw requested review from jleightcap and di August 1, 2023 20:10
@woodruffw woodruffw marked this pull request as ready for review August 1, 2023 20:10
@woodruffw
Copy link
Member Author

CC @steiza: I didn't change anything with this PR, but I think we'll eventually want to replace the skip-signing option with this (or, perhaps, just rewrite it in terms of xfail).

@steiza
Copy link
Member

steiza commented Aug 1, 2023

Yeah, on one hand we don't want to have to add a set of flags for every possible subset of functionality a client library supports, and supporting xfail is way easier than maintaining a set of flags.

On the other hand, when a client library updates to a new release of conformance testing, they'll have to manually go through any new tests that are failing, to determine if the failure indicates a bug or is using a feature their library doesn't support. Hopefully they are careful in their analysis!

fnmatch syntax is an interesting idea, especially for features that are contained in a single test file. But if we start to have features that overlap and try to name our functions to reflect that overlap... it would probably be easier to maintain flags.

After thinking through it, I'm okay with replacing skip-signing with xfail (or rewriting it to use xfail).

Signed-off-by: William Woodruff <[email protected]>
@woodruffw
Copy link
Member Author

Yeah, I think rewriting it to use xfail makes the most sense for now: signing is a "big" subset of functionality, so keeping a distinct option for it makes sense to me as a special case 🙂

Copy link
Contributor

@jleightcap jleightcap left a comment

Choose a reason for hiding this comment

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

LGTM for use with sigstore-rs.

@woodruffw woodruffw merged commit 95d2384 into main Aug 1, 2023
3 checks passed
@woodruffw woodruffw deleted the ww/xfail branch August 1, 2023 21:02
@woodruffw
Copy link
Member Author

I'll do a follow-up with fnmatch support.

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.

Add a flag for skipping bundle tests Add ability to feature-flag which tests are expected to fail
3 participants