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.

Sign up for GitHub

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

Jump to bottom

Fix and restructure functional tests #343

Merged
merged 17 commits into from
Dec 6, 2024
Merged

Fix and restructure functional tests #343

merged 17 commits into from
Dec 6, 2024

Conversation

Deezzir
Copy link
Contributor

@Deezzir Deezzir commented Nov 15, 2024 •
edited
Loading

  • Add a test to check if snaps are installed
  • Fixes some bugs in the functional test suit
  • Removes duplicate build step in the functional test (now the artifact build during the check workflow is used)
  • Restructure the functional tests to represent the current logic involving hardware-exporter.
  • Add a test to check if the charm manages to install the Nvidia drivers, if --nvidia flag is provided.
  • Add a --realhw for the real hardware tests.

The hardware-independent tests were built assuming the hardware-exporter service and the config would still be rendered if no hardware was detected. This is not the case anymore, so it limits the number of tests we can run in the CI. But I've confirmed that the tests are passing on real hardware with the changes.

Closes: #351

@Deezzir Deezzir requested a review from a team as a code owner November 15, 2024 02:16
@Deezzir Deezzir marked this pull request as draft November 15, 2024 02:17
@Deezzir Deezzir force-pushed the smartctl-snap-func-test branch 5 times, most recently from f35894d to 58da7c0 Compare November 20, 2024 02:19
@Deezzir Deezzir changed the title Add a func test to check if smartctl-exporter snap is being installed Fix and restructure functional tests Nov 20, 2024
@Deezzir Deezzir force-pushed the smartctl-snap-func-test branch from 58da7c0 to 63f5603 Compare November 20, 2024 02:23
@Deezzir Deezzir marked this pull request as ready for review November 20, 2024 02:37
rgildein
rgildein reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

I did not test it out, but I think it's should be fine. Only one comment for improvment.

tests/functional/conftest.py Outdated Show resolved Hide resolved
@Deezzir Deezzir force-pushed the smartctl-snap-func-test branch from 8791dfd to c029a5e Compare November 21, 2024 23:20
@Deezzir Deezzir force-pushed the smartctl-snap-func-test branch from c029a5e to edd1c0a Compare November 21, 2024 23:30
rgildein
rgildein previously approved these changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

LGTM

gabrielcocenza
gabrielcocenza previously approved these changes Nov 25, 2024
View reviewed changes
@Deezzir Deezzir marked this pull request as draft November 26, 2024 18:07
@Deezzir Deezzir dismissed stale reviews from gabrielcocenza and rgildein via ac14150 November 29, 2024 21:56
@Deezzir Deezzir force-pushed the smartctl-snap-func-test branch from bd1daeb to 2422ecb Compare November 29, 2024 22:27
@Deezzir Deezzir marked this pull request as ready for review November 30, 2024 02:56
rgildein
rgildein approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

LGTM, I tested it out and it's working as expected. I did not run all tests, but this PR is not changing them, so it should be fine.

samuelallan72
samuelallan72 previously approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@samuelallan72 samuelallan72 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - I also didn't run the tests, but it looks reasonable.

@samuelallan72 samuelallan72 dismissed their stale review December 3, 2024 23:04

Actually I shouldn't add approval without testing, so I'll withdraw my vote and simply comment. :)

gabrielcocenza
gabrielcocenza approved these changes Dec 4, 2024
View reviewed changes
tests/functional/conftest.py Show resolved Hide resolved
@Deezzir Deezzir merged commit 40d793d into canonical:main Dec 6, 2024
10 checks passed
@Deezzir Deezzir deleted the smartctl-snap-func-test branch December 6, 2024 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgildein rgildein rgildein approved these changes

@gabrielcocenza gabrielcocenza gabrielcocenza approved these changes

@samuelallan72 samuelallan72 samuelallan72 left review comments

@Pjack Pjack Awaiting requested review from Pjack Pjack is a code owner automatically assigned from canonical/soleng-reviewers

@aieri aieri Awaiting requested review from aieri aieri is a code owner automatically assigned from canonical/soleng-reviewers

@jneo8 jneo8 Awaiting requested review from jneo8 jneo8 is a code owner automatically assigned from canonical/soleng-reviewers

@chanchiwai-ray chanchiwai-ray Awaiting requested review from chanchiwai-ray chanchiwai-ray is a code owner automatically assigned from canonical/soleng-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Functional test will timeout because of the Nvidia drivers
5 participants
@Deezzir @samuelallan72 @jneo8 @gabrielcocenza @rgildein