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 testbench #39

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

tibersam
Copy link
Collaborator

This PR implements some parts of #29.
It adds three tests:

  • Unittest of analysisfunctions used in other tests
  • Test of different inputs into python worker
  • Test of the controller running

This PR does not implement all of the features of #29 and is more thought as a start of adding automated testing to ARCHIE

@tibersam tibersam force-pushed the add_testbench branch 2 times, most recently from 9c46583 to a1ed441 Compare February 14, 2022 12:15
@tibersam
Copy link
Collaborator Author

@lukasauer and @aewag can you review this?

from hdf5logger import hdf5collector


def compare_hdf5_files():
Copy link
Member

Choose a reason for hiding this comment

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

Locally, I used h5diff two compare an output vs the golden hdf5 file.

Can we also use this here (or a similar tool)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no experience with h5diff. What i want to avoid is a failed test if the experiment index numbers change. Can h5diff cope with it?

Copy link
Member

@aewag aewag Feb 15, 2022

Choose a reason for hiding this comment

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

It will most likely print an error as the files don't match. Does this happen as well with only a single runner? I would have expected that the output of ARCHIE should be deterministic in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will not use h5diff, as it is not python native and i want to use python assert. However i will use the same trick i am using to compress the collected analysis data from the experiment datas and check if it is empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now the testbenches also assert if indexing stays consistant by asserting the experiment group title and testing extracted index numbers.

Copy link
Member

@lukasauer lukasauer left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! A couple of additional things I noticed:

The test take quite a long time to run. Is there a way to reduce this? Perhaps by reducing the number of faults? Ideally we would want something that can also be run locally to check if something broke.

Your tests use the stm32f0discovery QEMU machine, which is only available in our downstream version of QEMU. I want to keep our patches on top of QEMU to minimum. It would therefore be great, if we can use a machine that is in upstream QEMU for the tests. For example stm32vldiscovery.

@@ -16,7 +16,7 @@ jobs:
uses: actions/checkout@v2

- name: install packages
run: sudo apt update; sudo apt upgrade -y; sudo apt install -y build-essential ninja-build libglib2.0-dev libfdt-dev libpixman-1-dev zlib1g-dev python3-tables python3-pandas python3-prctl
run: sudo apt update; sudo apt upgrade -y; sudo apt install -y build-essential ninja-build libglib2.0-dev libfdt-dev libpixman-1-dev zlib1g-dev python3-tables python3-pandas python3-prctl python3-pytest python3-pytest-cov python3-deepdiff
Copy link
Member

Choose a reason for hiding this comment

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

We might also want to add these to a new requirements file (tests/requirements.txt perhaps) so it's easier to get the test running locally. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, i will add a new requirements like you proposed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Is this resolved? If yes, I think we can remove these here and link to the requirements file? BTW I didn't see deepdiff in the requirements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deepdiff is no longer needed. I have pivoted to another solution. It is resolved from my perspective.

from faultclass import python_worker, Fault


class dummy_args:
Copy link
Member

Choose a reason for hiding this comment

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

I think test_args would be a better name here

[Fault(134217728, 0, 2, 1000, 3, 134217950, 1, 0)],
"tests/testdata/worker_data.xz",
),
([Fault(3, 2, 1, 0, 4, 134217954, 1, 0)], "tests/testdata/worker_register.xz"),
Copy link
Member

Choose a reason for hiding this comment

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

nit: Please use the same format as for the other entries

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This formatting was introduced by black, i can add an ignore around it if you want.

)
def test_python_worker(fault, comparedata):
parguments = pythonworker_enviroment()
# print(parguments)
Copy link
Member

Choose a reason for hiding this comment

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

nit: please remove commented out code, here and in the other files as well

@tibersam
Copy link
Collaborator Author

The test take quite a long time to run. Is there a way to reduce this? Perhaps by reducing the number of faults? Ideally we would want something that can also be run locally to check if something broke.

The tests take most of their time in the comparing of the results. I will look into how to speed up the process, probably using h5diff. Also it can be speed up by using the -n option of pytest, aka running the test locally in parallel.

Your tests use the stm32f0discovery QEMU machine, which is only available in our downstream version of QEMU. I want to keep our patches on top of QEMU to minimum. It would therefore be great, if we can use a machine that is in upstream QEMU for the tests. For example stm32vldiscovery.

I can understand this, however the miniblink currently is the default binary we ship with archie and use to verify that the installation worked. Therefore i decided to use it for the tests.

@tibersam
Copy link
Collaborator Author

tibersam commented Mar 3, 2022

The test take quite a long time to run. Is there a way to reduce this? Perhaps by reducing the number of faults? Ideally we would want something that can also be run locally to check if something broke.

Currently all 6 tests introduced with this PR take ~90s on my laptop. Coverage tracing increases the duration of the tests to ~170s. The controller test takes the longest with ~65s, analysisfunction takes ~5s, and python_worker takes ~17s. All times taken without running tests in parallel. If you look into the github runner in my repository all test take on the github vm ~ 4m30s. I think that these test can be run locally without long delays. Also if you specify which testbench to run you can for example exclude the controller test until you have finished the change.

@tibersam tibersam requested a review from lukasauer March 9, 2022 12:44
@tibersam tibersam requested a review from aewag March 9, 2022 20:55
Copy link
Member

@aewag aewag left a comment

Choose a reason for hiding this comment

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

conftest.py: For what is the empty file needed?

@@ -16,7 +16,7 @@ jobs:
uses: actions/checkout@v2

- name: install packages
run: sudo apt update; sudo apt upgrade -y; sudo apt install -y build-essential ninja-build libglib2.0-dev libfdt-dev libpixman-1-dev zlib1g-dev python3-tables python3-pandas python3-prctl
run: sudo apt update; sudo apt upgrade -y; sudo apt install -y build-essential ninja-build libglib2.0-dev libfdt-dev libpixman-1-dev zlib1g-dev python3-tables python3-pandas python3-prctl python3-pytest python3-pytest-cov python3-deepdiff
Copy link
Member

Choose a reason for hiding this comment

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

Is this resolved? If yes, I think we can remove these here and link to the requirements file? BTW I didn't see deepdiff in the requirements.

@tibersam
Copy link
Collaborator Author

conftest.py: For what is the empty file needed?

I have written the reason in the readme.
The Problem: If you call pytest (not python3 -m pytest) the Archie folder is not added to the search path of the python interpreter. However if it finds the conftest.py pytest adds the folder it is contained in to the search path. Why this is not the default of pytest is beyond me.

conftest.py is used to setup common fixtures needed by the different test benches, currently it is not used as this but might be in the future. We currently need it to "hack" the searchpath problem...

@aewag
Copy link
Member

aewag commented Mar 10, 2022

@lukasauer and me just discussed that we would like to keep this PR for now open. We have some more local changes and todos we would like to solve prior to merging this. One major change is to rebase ARCHIE on top of the newest QEMU version, which introduces some breaking changes and which is already prepared by Lukas.
So it probably makes sense to wait until this has landed and after that progress further with this PR.

@tibersam
Copy link
Collaborator Author

@aewag Cool. Then I will wait. Do you have a time frame when you will update? Like in a week, in a month, in two months ...?

@lukasauer
Copy link
Member

We are waiting for QEMU 7.0 to be released, which should be soon (currently it's at rc1).

@tibersam
Copy link
Collaborator Author

@lukasauer Do you have a timeline when you will update the project?

@lukasauer
Copy link
Member

Sorry for the delay, this is currently planned for next month.

@tibersam
Copy link
Collaborator Author

@lukasauer Sorry for the long absence. When PR #55 passes, should the test bench use both examples instead of miniblink for testing? I would then try to rework the testbench the coming weeks to reflect the changes introduced since this PR was opened.

@aewag
Copy link
Member

aewag commented Feb 14, 2023

@lukasauer Sorry for the long absence. When PR #55 passes, should the test bench use both examples instead of miniblink for testing? I would then try to rework the testbench the coming weeks to reflect the changes introduced since this PR was opened.

We have currently more changes internally. I think it may be best to wait for the rework until they are landed.

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