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

Regression test for the parameters generated by the app #644

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

superstar54
Copy link
Member

@superstar54 superstar54 commented Mar 21, 2024

In order to implement #601 .

The previous test used the final builder generated by the workchain, which is like an end-to-end test. However, the builder depends on the version of the aiida-quantumespresso, which we want to avoid for the moment. In this PR, we only test the ui_parameters generated by the app. This ui_parameter is the most important parameter, and it contains all the information on the job. We can use it to recreate the whole qeapp instance. The test here guarantees that the ui_parameters are the same as the reference data.

The previous test used the final builder generated by the workchain, which is like an end-to-end test. However, the builder depends on the version of the aiida-quantumespresso, which we want to avoid for the moment.  In this PR, we only test the ui_parameters generated by the app. This ui_parameter is the most important parameter, and it contains all the information on the jobs. We can use it to recreate the whole qeapp instance. The test here guarantees that the ui_parameters are the same as the reference data.
@superstar54 superstar54 force-pushed the regression_ui_parameters branch from adbb972 to a3c35ac Compare March 21, 2024 10:14
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.85%. Comparing base (ed72b08) to head (4ff784d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   75.78%   75.85%   +0.07%     
==========================================
  Files          60       60              
  Lines        4299     4312      +13     
==========================================
+ Hits         3258     3271      +13     
  Misses       1041     1041              
Flag Coverage Δ
python-3.10 75.85% <100.00%> (+0.07%) ⬆️
python-3.9 75.88% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Cool! I didn't realize you have these tests, might steal them for my app as well. 😊

I didn't check all the details but the overall approach and motivation makes sense to me.

@superstar54 superstar54 enabled auto-merge (squash) March 21, 2024 16:23
@superstar54 superstar54 disabled auto-merge March 21, 2024 16:23
@superstar54 superstar54 merged commit a6f3517 into aiidalab:main Mar 21, 2024
7 checks passed
@unkcpz
Copy link
Member

unkcpz commented Mar 21, 2024

Emmm.... I got the idea to remove the builder generate part, but instead of removing it entirely it should be some where else.
The initial implementation of data-regression test test two things, 1) The parameters are correctly passed from UI to widget 2) the parameters get by widgets can generate to the desired builder.
In this PR you remove the test for the second part. It actually useful to spot things like the parameters are correctly overwritten, I remember it used to failed for some PR of @AndresOrtegaGuerrero and help us to find the issue of parameters passing.

Since this PR is merged, I'd suggest to bring the workflow test back, since the idea is borrow from aiida-quantumespresso to test the workchain. We implement the workchain here as well and having some logic inside, so make no sense to me to just remove it entirely.

@superstar54
Copy link
Member Author

superstar54 commented Mar 21, 2024

Since this PR is merged, I'd suggest to bring the workflow test back, since the idea is borrow from aiida-quantumespresso to test the workchain. We implement the workchain here as well and having some logic inside, so make no sense to me to just remove it entirely.

I agree that the test on the WorkChain part is also important. I opened an issue on this, #645. I think it's meaningless to move it somewhere else and disable the test. The test itself needs to be rewritten completely.

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