-
Notifications
You must be signed in to change notification settings - Fork 4
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
Create core Agent integration tests #231
Conversation
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.
Very interesting! Comments below pertain to the work as a whole, rather than this add-on.
I ran into docker conflicts when trying to run these ... would it help to give fixed container names that were a little less generic? I already had an "influxdb" running ... presumably these could all be called ocs-tests-influxdb and so on?
I had intermittent test failures for:
test_fake_data_after_crossbar_restart
test_aggregator_after_crossbar_restart
To debug I tried to docker-compose up in the tests directory and look at logs; at that point I realized the crossbar entry in default.yaml was causing fake-data-1 to not start up (#224). Can that just be removed from default.yaml here?
test_crossbar_integration.py is full of hard-coded sleeps. I think those intervals should be declared as variables at the top of the script -- i.e. the time to wait for agents to come back online; the time to wait for an agent to exit due to crossbar downness.
The "no gaps in the data" test was failing for me (I think I saw you'd commented this out in the other PR, or something?). Instead of looking at timestamps, you could invent a data stream in fake_data_agent that simply records an incrementing counter [0, 1, 2 ...]. That's very easy to check for problems!
57a7807
to
6df7581
Compare
Mhm, good point. I've updated container names and ports to avoid conflicts.
Strange, I removed the crossbar entry in
I've moved the wait time before crossbar comes up. The other time out, waiting for an Agent to exit only exists in one test, so I'll leave it locally for now.
Mhm, I like that idea. But yeah, that test is not good. That final assert is commented out now, probably I should remove more of that test. It came about after I saw some behavior that isn't reliably reproduced (though I didn't realize that at the time.) On a related note, I've removed some of the other tests which aren't really needed, like the |
This env var was being retained between tests otherwise.
Following the second recommendation in [1]. Note we avoid the pitfall the documentation points out since we run our tests from the tests/ directory and not from the root of the repo. [1] - https://docs.pytest.org/en/6.2.x/goodpractices.html#tests-outside-application-code
Note this was squashed from many debugging commits to trigger the CI workflow and debug workflow issues.
Also clean up some of the debugging in the github workflow.
Also session scope integration tests that depend on so3g.
The Client fixture now attempts reconnects up to a certain timeout, which is a faster way of waiting for the Agent to come up, since we can just check rapidly, rather than way some time that fluctuates given the load of the system for the Agent to finish starting up.
If an OCS system is already running on the machine running the tests we would collide with in use ports and container names. This avoids that, allowing tests to run on the same system.
`test_testing` was just neede initially when figuring out the wait_for_crossbar fixture. And `test_influxdb_publisher_after_crossbar_restart` is just a pass at this time. Commenting these out will save some time.
f3a9027
to
39122f0
Compare
Had to rebase and force push (after rebasing the unit test branch...learning a lesson at the moment...) Will merge when checks complete. |
From mistyped keyboard shortcut.
Description
This creates integration tests for the Tasks and Processes for the core OCS Agents. The Host Master Agent still lacks a bit of testing, however. This is building on the work in #230 on unit tests. (Since this builds on #230 I've PR'd onto that branch as well for ease of reading. #230 should be merged first, this'll redirect to
develop
then.)The early integration tests that tested robustness to crossbar server reboots relied on pytest-docker-compose, which starts up a set of docker containers running Agents and other components needed on the network (i.e. the crossbar server, InfluxDB). We've reduced the number of containers these tests need, and in the future will continue to do so.
The CI pipeline will run almost all of these tests. There is a small set of integration tests that depend on a dockerized crossbar container and spt3g/so3g, which is a complicated environment to setup on GitHub actions at the moment due to the need to either install spt3g+so3g (long compile times) or run within and so3g based container (where we can't use pytest-docker-compose). There are several paths forward there, and I leave that for later PRs while noting the coverage missing from these tests is minimal.
Motivation and Context
This is largely motivated by the need for automated testing, working towards #28. The fact that this starts up every core Agent and interacts with them via a Client means we're largely there too. With some additional coverage of important parts of the core library I think we'll be ready to close #28 soon.
How Has This Been Tested?
I ran the complete test suite, locally, in an environment that has Docker/Docker Compose and spt3g/so3g:
Types of changes
Checklist:
develop
branch.