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

Integration tests #4

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

SimeonEhrig
Copy link
Member

@SimeonEhrig SimeonEhrig commented Jul 6, 2023

The PR adds the tool integTestGen.

The integTestGen.jl creates jobs for the CI, which checks if the changes in PR does a API or behavior break which causes that other QED packages which use the package of PR does not work anymore.

A detailed documentation is part of the PR. The doc also provides the documentation for the unit tests, introduced in PR #3.

@SimeonEhrig SimeonEhrig force-pushed the integrationTests branch 4 times, most recently from 092607c to 7a6dfd5 Compare July 13, 2023 06:54
@SimeonEhrig
Copy link
Member Author

Testing the PR

To test the PR, we need an unconventional setup. I tested the PR with the QED packages QEDbase and QEDfields. QEDfields depends on QEDbase. If QEDbase introduce a API break, QEDfields will not work anymore.

For testing purpose, I introduce a API break in QEDjl-project/QEDbase.jl#3. The CI of the commmit fails because QEDfields does not work with the change. On the this branch I provide a fix for QEDfields. When I set the custom URL via commit message, the test passes. Now we could merge the API break PR of QEDbase and directly open a PR with the fix for QEDfields and merge it.

@SimeonEhrig SimeonEhrig marked this pull request as ready for review July 13, 2023 07:08
@SimeonEhrig
Copy link
Member Author

@szabo137 Ready for review

Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

General feedback for the script:

mutable global variables should be used with caution, especially if this causes a dependence of a function on the state of the global scope. This may cause unpredictable behavior.

One solution, which is not perfect but better, is the passing of the global variable to the local scope of functions, i.e. passing them as an argument.

ci/integTestGen/README.md Outdated Show resolved Hide resolved
ci/integTestGen/README.md Outdated Show resolved Hide resolved
ci/integTestGen/README.md Outdated Show resolved Hide resolved
ci/integTestGen/README.md Outdated Show resolved Hide resolved
ci/integTestGen/src/integTestGen.jl Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
@szabo137
Copy link
Member

Maybe consider renaming the directory ci into a hidden folder .ci. This is more convenient and goes in line with other meta-functionality like the .formatting (as present in QEDbase.jl).

@tjungni
Copy link
Member

tjungni commented Jul 20, 2023

Could the first box in docs/src/integration_jobs_pipeline.svg be called "Stage: Test modified subpackages"?
As far as I understood this stage isn't integration yet

docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
@SimeonEhrig
Copy link
Member Author

@tjungni I rewrote the Integration Tests for CI Develops section. Can you please check it again. I hope it is better understandable now.

Copy link
Member

@tjungni tjungni left a comment

Choose a reason for hiding this comment

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

A few more small edits and some questions

.ci/integTestGen/src/integTestGen.jl Outdated Show resolved Hide resolved
docs/src/ci.md Show resolved Hide resolved
docs/src/ci.md Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
docs/src/ci.md Outdated Show resolved Hide resolved
@SimeonEhrig SimeonEhrig force-pushed the integrationTests branch 2 times, most recently from a08134a to 1a3547c Compare July 31, 2023 12:51
@SimeonEhrig
Copy link
Member Author

@tjungni Ready for merge?

docs/src/ci.md Outdated Show resolved Hide resolved
Copy link
Member

@tjungni tjungni left a comment

Choose a reason for hiding this comment

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

Ready to merge

Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

Every request was correctly addressed—no concerns from my side.

Ready for merge!

@szabo137 szabo137 merged commit 26a346e into QEDjl-project:dev Aug 21, 2023
@AntonReinhard AntonReinhard added this to the Release-0.1.0 milestone Aug 30, 2024
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.

4 participants