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

skip tests run as "non-packaged" (non-rpm) validation #353

Merged
merged 7 commits into from
Jun 24, 2024

Conversation

Vishwanatha-HD
Copy link
Contributor

Added changes to skip the dotnet regular tests which are not required to run during cross build SDK validation.

@Vishwanatha-HD
Copy link
Contributor Author

@omajid. please review my changes and merge it, if the changes are fine.

@@ -7,7 +7,8 @@
"type": "bash",
"cleanup": true,
"skipWhen": [
"vmr-ci" // bash completion script not installed
"vmr-ci", // bash completion script not installed
"non-packaged",
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to this line (sort of like what is in the previous line) to indicate why we are skipping this? Same goes for other json files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@omajid, I have added the comment for all the files to specify the reason for skipping the test. Please review the same. thanks..

openssl-alpn/test.json Outdated Show resolved Hide resolved
restore-with-rid/test.json Outdated Show resolved Hide resolved
@@ -7,6 +7,7 @@
"type": "bash",
"cleanup": true,
"skipWhen": [
"non-packaged", // skip during cross-bld-sdk/tarball validation
Copy link
Member

Choose a reason for hiding this comment

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

This test is checking if we're applying a certain patch for RHEL9.
I wonder why this test does not fail on hydra's vmr builds (since they don't include the patch) 🤔
We should take a closer look.

cc @nicrowe00

system-openssl/test.json Outdated Show resolved Hide resolved
@tmds
Copy link
Member

tmds commented Jun 12, 2024

// skip during cross-bld-sdk/tarball validation

@Vishwanatha-HD this doesn't describe why the test fails. I've added some questions as I like to understand better how the test fail in the "non-packaged" environment?

@Vishwanatha-HD
Copy link
Contributor Author

Hi @tmds..
Omair and me had a discussion about the failures and please find his explanation below on why the tests fail when we try to validate the cross-build SDKs.. We mean "non-packaged" when we try to validate just the tarball and not the complete RPMs..

The errors fall into a few buckets:

Failing tests >>>>
-> publish-aspnet-selfcontained
-> publish-dotnet-selfcontained
-> restore-with-rid
-> openssl-alpn
-> system-openssl:

  • The above tests fail because they expect to be testing a source-built SDK and a cross-built SDK is different enough
    • For example, doesn't know about rhel.9-s390x RID
    • The cross-built SDK is portable, and doesn't link to some system libraries that the tests want to poke through

Failing tests >>>>
-> bash-completion
-> man-pages
-> tools-in-path

  • The above tests fail because they expect to be testing a packaged SDK, which has features like bash-completion enabled, which isn't a part of the .NET SDK tarball.

Failing tests >>>>
-> file-permissions
-> rsa-pkcs-openssl
-> sha1-validation

  • The above tests fail because they are testing Red Hat-specific changes, such as whether
    omajid/dotnet-runtime@076687f
    has been applied to the SDK.

Please let me know if you need more info and we will able to provide them.. Thanks..

@tmds
Copy link
Member

tmds commented Jun 12, 2024

why the tests fail when we try to validate the cross-build SDKs..

fyi, in hydra CI we don't run the regular tests against the cross-built SDKs. The cross-built SDK jobs provide SDKs for native builds. And those builds run the tests.

For example, doesn't know about rhel.9-s390x RID

The cross-built SDK for .NET 9 gets built with a rid of linux-390x and provides assets under that rid. Rather than skipping these tests, they can run if they are using the appropriate SDK rid.

The cross-built SDK is portable, and doesn't link to some system libraries that the tests want to poke through

Can you be explicit about this in the comments? I don't think non-packaged conveys these tests are skipped because the build is portable. Perhaps you could use something else, like skipWhen: portable.

Failing tests >>>>

For these other tests, which should be ones that already have vmr-ci but where we are now adding now for the same reason: non-packaged.

It would be nice if we can avoid this duplication by either using vmr-ci or non-packaged, but not add them both.

For the managed-symbols-available test you can either also extract the symbols tarball so the test would pass, or add a new dedicated trait like: skipWhen: no-symbols.

Note that the test runner can be given multiple traits to skip, so you can set it to skip test matching any of: vmr-ci/non-packaged, portable, and no-symbols.

@Vishwanatha-HD
Copy link
Contributor Author

@tmds and @omajid,
I have addressed all the code review comments and made the corresponding changes.
Please take a look and merge the CL if its fine.. thanks

@omajid
Copy link
Member

omajid commented Jun 18, 2024

This is great! Thank for you this really nice cleanup!

system-openssl/test.json Outdated Show resolved Hide resolved
sha1-validation/test.json Outdated Show resolved Hide resolved
restore-with-rid/test.json Outdated Show resolved Hide resolved
openssl-alpn/test.json Outdated Show resolved Hide resolved
omajid added 3 commits June 21, 2024 12:41
And run the test everywhere. There's no reason to skip it for portable
builds.
There's no reason to skip this test for portable, cross or other types
of builds.
No reason this test shouldn't work for portable SDKs.
@Vishwanatha-HD
Copy link
Contributor Author

@omajid, I took a look at your changes to run the testcase with SDK runtime-ids rather than the RPM ones.. The changes are looking good.. Thanks for doing this for me.. !!

@Vishwanatha-HD Vishwanatha-HD requested review from omajid and tmds June 24, 2024 09:03
@omajid omajid merged commit 5a92120 into redhat-developer:main Jun 24, 2024
12 of 17 checks passed
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.

3 participants