-
Notifications
You must be signed in to change notification settings - Fork 28
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
Test init_pipeline outside of the test process and drop the (partial) database if the initialisation fails #165
base: main
Are you sure you want to change the base?
Conversation
298e451
to
6ce82b2
Compare
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.
This PR has been open so long that peekJob got added after it was submitted. Perhaps update the documentation here to indicate that flags can be passed through to _test_ehive_script
as well
6ce82b2
to
10a8c0f
Compare
Done. I have rebased the branch and added the doc for |
init_pipeline was the last script tested this way. That caused some issues because it is not isolated from the test process, and for instance it will register the pipeline in TheApiary. Also, the test is more reliable if the actual script is tested.
10a8c0f
to
28d80c6
Compare
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
- Coverage 82.79% 82.78% -0.02%
==========================================
Files 184 184
Lines 10557 10564 +7
Branches 1660 1668 +8
==========================================
+ Hits 8741 8745 +4
+ Misses 1126 1122 -4
- Partials 690 697 +7
Continue to review full report at Codecov.
|
…database It will be created automatically
I have added there a few commits to deal with dropping the database when |
Use case
init_pipeline
was the last script tested in the test process itself. A long time ago, I thought it would simplify testing, but I gradually realised it's causing more problems because it is not isolated from the test process. All test functions have since been moved to calling the script withsystem
andinit_pipeline
was the last one to work the old way.One problem is that it loads all the Runnables and registers the pipeline in TheApiary. And since the main script was not tested, some tests were explicitly not using the test function in order to test the script.
The other thing I'm addressing here is the ability to expect script failures and to test the error message.
If there is say a syntax error in a Runnable,
init_pipeline.pl
rightly detects it:However, this leaves the database in a partial state and I need to add
-hive_force_init 1
once I have fixed the Runnable.Description
1/2. I have reimplemented the test
init_pipeline
function to use_test_ehive_script
, which now has an extra parameter named$flags
with test options. At this time the only option isexpect_failure
which is either 1 or the regular expression to match the script's standard error against.3. I made
init_pipeline.pl
drop the database if the initialisation is not complete, which makes it quicker for the user.Possible Drawbacks
Perhaps some users manipulate TheApiary in tests after calling
init_pipeline
. They would have to add a line of code to load the pipeline from the database.Testing
Have you added/modified unit tests to test the changes?
Yes
If so, do the tests pass/fail?
Yes
Have you run the entire test suite and no regression was detected?
Yes