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

Dcuc testing #13

Merged
merged 14 commits into from
Oct 29, 2024
Merged

Dcuc testing #13

merged 14 commits into from
Oct 29, 2024

Conversation

macpijan
Copy link
Contributor

@macpijan macpijan commented Aug 5, 2024

@BeataZdunczyk @mkopec @philipandag Please use tests and implement new ones for new features.
We are already missing tests for the most recently added feature.

Testing is documented in the README: https://github.com/Dasharo/dcu?tab=readme-ov-file#testing

As a followup, we need following improvements:

Signed-off-by: Maciej Pijanowski <[email protected]>
Signed-off-by: Maciej Pijanowski <[email protected]>
@macpijan macpijan requested review from mkopec and philipandag August 5, 2024 19:23
@philipandag
Copy link
Contributor

philipandag commented Aug 6, 2024

@macpijan
The tests for mac subcommand are in a weird situation, because we have not yet published a binary that supports them. To use the command, the binary has to have a valid gbe section with a valid checksum of the mac address but the binaries we have on cloud have the section filled with FF bytes, so the checksum test fails. Should I just prepare a rom that works and push it to dcu repo?

@macpijan
Copy link
Contributor Author

macpijan commented Aug 7, 2024

The tests for mac subcommand are in a weird situation, because we have not yet published a binary that supports them.

NovaCustom MTL releases do not support this? I thought this was the tool for the non-GPU models, which have been released already?

@philipandag
Copy link
Contributor

philipandag commented Aug 7, 2024

NovaCustom MTL releases do not support this? I thought this was the tool for the non-GPU models, which have been released already?

To work on these binaries the GBE section has to be first replaced with a valid, blank one. We have the gbe blob available to download in our repo:
https://github.com/Dasharo/dasharo-blobs/blob/mtl-h/novacustom/v5x0tu/gbe.bin

The tests could download both the binary and gbe section and then replace it in the binary to perform tests.

@mkopec
Copy link
Member

mkopec commented Aug 7, 2024

The iGPU releases shipped without a GbE region @macpijan . The blob was added to the repo after the release and DCU requires that blob to be in the binary.

We can inject the blob into the binary after the fact and provide it somewhere publicly (we probably don't want to replace the original binaries, but provide "full" binaries alongside them)

@macpijan
Copy link
Contributor Author

macpijan commented Aug 7, 2024

The tests could download both the binary and gbe section and then replace it in the binary to perform tests.

That sounds ok to me.

@philipandag philipandag self-requested a review August 9, 2024 07:03
@philipandag
Copy link
Contributor

I have added tests for mac command. There is still the issue that approvals show absolute paths which will look different every time someone runs the approvals. Are we fixing that?

@philipandag
Copy link
Contributor

We are using an old version of approve.bash. The new version allows for automatically accepting some differences as @macpijan suggested:

There is an option to allow for some diff (such as local paths): https://github.com/dannyben/approvals.bash?tab=readme-ov-file#allowing-some-difference

I have tried using this option with $PWD which caused sed, which is used to replace the allowed string with * , to crash. Then I tried passing escaped $PWD (${PWD//\//\\/}) but that just did not make any difference.

@philipandag philipandag linked an issue Aug 20, 2024 that may be closed by this pull request
@philipandag philipandag self-requested a review October 14, 2024 11:10
@philipandag
Copy link
Contributor

With the latest commits a CI worflow was added. It was tested to work locally using https://github.com/nektos/act but I can see it does not on the remote. It needs some debugging.

@philipandag philipandag force-pushed the dcuc-testing branch 2 times, most recently from c848874 to 3c02eba Compare October 14, 2024 14:34
@philipandag
Copy link
Contributor

Looks like the CI is now running fine although the logs don't look like the tests are actually performed, like it ends just after downloading the files needed for the tests...

test/test-data.sh: Add preparation of data for MAC command
test/approvals: Add MAC approvals

Signed-off-by: Filip Gołaś <[email protected]>
test/approvals/: Update to approvals not containing the PWD

Signed-off-by: Filip Gołaś <[email protected]>
@philipandag philipandag force-pushed the dcuc-testing branch 2 times, most recently from c970fd1 to 6cb6139 Compare October 15, 2024 07:13
philipandag and others added 3 commits October 15, 2024 09:42
test: Prepare for using in GH CI
test/approvals: Add approvals for CI
.github/workflows/tests.yml: Add CI workflow for testing dcuc

Signed-off-by: Filip Gołaś <[email protected]>
@philipandag philipandag force-pushed the dcuc-testing branch 2 times, most recently from ac77610 to 9cc20e7 Compare October 15, 2024 09:39
@philipandag philipandag force-pushed the dcuc-testing branch 2 times, most recently from c68f380 to 6a1f81d Compare October 21, 2024 06:57
@mkopec mkopec merged commit ed3adfa into main Oct 29, 2024
2 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.

Missing tests for the MAC command
3 participants