-
Notifications
You must be signed in to change notification settings - Fork 74
[#697] Github actions to run all PRC tests #772
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
Conversation
9bd1449 to
19ef753
Compare
fa0b1ae to
5cfcce8
Compare
|
All the checks implemented via github actions (except Codacy) are passing . Some cleanup has been done on the code as well. I think we're for a preliminary round of review. Just be aware there are some extra scripts that are not central to the github actions we've implemented here. They are there as examples and tests of the harness functionality. |
|
please mark the ones that are 'not central'. are you suggesting we keep the scripts that are 'not central'? if so, why? |
They're illustrative but not necessary, so I'll remove them. |
Ah, okay - yes please remove them. And consider putting any helpful words / examples in README.md files at the appropriate locations. |
08a524c to
26cc9a1
Compare
|
We are now down to 6 basic commits. |
f14118d to
e2f2615
Compare
|
We're now down to 8 basic commits. If that's too many for the convenience of those reviewing, let me know and I can maybe digest it down further. |
|
8 commits isn't bad at all, but let's go ahead and squash commits into their final shape if we know what that is. |
e2f2615 to
a395ff6
Compare
|
Commits now minimized to 2. |
irods/test/scripts/test010_issue_362_rogue_chars_in_pam_password.bats
Outdated
Show resolved
Hide resolved
irods/test/scripts/test010_issue_362_rogue_chars_in_pam_password.bats
Outdated
Show resolved
Hide resolved
|
Reverted all experimental changes and things are back to normal for final round of commentary and review.... |
|
please squash so this is a candidate for merging. i still see two extra commits i think. and consider renaming |
|
My own editorial on it, keep them separate for proper attribution. And, because there is fundamentally different work being done:
|
|
My top suggestion at this point is to revert / delete that last commit. Why rename setupssl.py? I think we should leave that alone since it's publicly available and documented as setupssl.py. If you want to rename it, let's make a new issue and discuss how to do that at another time... unless you need to rename it in order for these changes to work. I think the "remove Jenkins test framework support" commit can be linked to #778, yes? Finally, looking at "[_TENTATIVE_SQUASH] mostly review based changes ..."... This one should be split up and squashed into the commit(s) with which each change is best suited. For example, it includes a change to get rid of all symlinks. I see one symlink that was introduced in "test harness and container-based tests", so that should be squashed into there. If no reasonable (I will let you decide what that means) split / squash can be had for these changes, maybe just keep it in its own commit and link it to all of the relevant issues. It's not ideal, but if it can't be done in a clean manner, maybe that's all we can do. I think the rest of the commits look fine. |
|
the rename ssl thing was me - i noticed all the other files had underscores in their naming... and it stood out. |
I can split it up pretty easily, as it still exists on another branch in unsplit form. |
I'll work on the merge-compatible squashing tonight and defer to consensus on the renaming or not of setupssl.py |
korydraughn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
684c2c5 to
2108494
Compare
|
Given the new changes and the way things are arranged now, I would make a best-effort attempt at squashing the commits without issue numbers into the commits with issue numbers. If it proves too messy/difficult to sort through, I (personally) am not opposed to "formatting" commits (which exclusively or primarily deal with whitespace) that come at the end of a line of commits. Leaving that up to you, @d-w-moore.
Looking into this further, I think it's okay to rename that script. It appears to be used internally for testing only. Please squash that into the commits with issue numbers as well. |
No problems, will do the squashing next. |
af5d51b to
b027915
Compare
korydraughn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pound it!
Co-authored-by: d-w-moore <dmoore@renci.org> Includes a docker compose configuration in which the PRC test suite can be run in a way representative of typical operational use, and the version of iRODS server and python interpreter that we install are also easily reconfigurable. Additionally, this affords us the opportunity to run the test suite on a client node all its own, the iRODS server being reachable only via the network.
A new test harness is introduced in which we construct a new container (using either Docker or podman) for each test program we run. This allows full customization of the container environment for the particular needs of each test. Accordingly, also included with the Github workflows is a full run of the PRC test suite with the iRODS server and catalog DB server running in the same container as the client. In the process of putting old tests through new rigors, faults were found and corrected in some of those tests.
The test now fetches the modify and access timestamps during the interval in which the replica is open, then asserts those timestamps as equal to each other, as well as that they are greater than a system-generated datetime of about two seconds prior. (This is due to some timestamps including a microseconds count whereas others do not.)
24d8f7e to
14a3895
Compare
|
The documentation introduced in this PR for running the tests also addresses #637. We will not be linking any specific commit to the issue, so I am doing it here in this comment. Merging. |
All tests will run from GitHub actions.
Except the interactive PAM test, and that omission will soon be rectified.