-
Notifications
You must be signed in to change notification settings - Fork 472
Re-implemented from scratch integration testing #1716
Conversation
They were buggy and we need to take another approach. Signed-off-by: Miquel Sabaté Solà <[email protected]>
As you can see I'm fighting Travis 😅 Review once I'm done. |
c9bd010
to
6e6ba94
Compare
@vitoravelino this is ready to be reviewed. Travis is passing, yay! 😁 |
require "portus/test" | ||
|
||
# TODO: on a Travis PR, only test portus:development and registry:2.{5,6} | ||
# TODO: otherwise, test with portus:2.3 and portus:head |
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.
@vitoravelino this has been left out on purpose. I will tackle this in a subsequent PR. The idea is that running the whole matrix takes around 40 minutes on Travis, which is simply unbearable for mere PRs. So, after merging this PR, I'll send another which:
- On PR it will just run the development image. In total it would then take around 20 minutes.
- If this is not a PR (e.g. a PR has just been merged and travis is performing a run against the fresh master branch), then we will test with 2.3 and head. This should take around 30 minutes.
If we see that overall we are taking too much time on Travis, then we could start thinking on parallelizing (e.g. running the integration tests on a Jenkins job instead of Travis).
# Ruby tests | ||
__database restart | ||
bundle exec rspec spec | ||
__database stop |
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.
Any specific reason to restart and then stop the database?
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 is because unit tests require the DB to be up, and integration tests raise their own database, so we should stop the main DB so there are no clashes.
bin/test-integration.sh
Outdated
for f in $TESTS; do | ||
tests+=("$ROOT_DIR/spec/integration/$f.bats") | ||
done | ||
fi |
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.
Replace tabs with spaces.
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.
Fixed
spec/integration/README.md
Outdated
test), and we are using a couple of helpers defined in | ||
`spec/integration/helpers.bash`: | ||
|
||
TODODOTODODODO |
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.
Leftover or incomplete?
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.
Fixed
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.
Kudos! 👍
This PR also fixes #1714, right? |
6e6ba94
to
59e19b1
Compare
I've also fixed a couple of small nitpicks in the current testing setup. Fixes SUSE#1667 Signed-off-by: Miquel Sabaté Solà <[email protected]>
59e19b1
to
f297fd7
Compare
In principle yes, but I still want to make sure that it works out of the box outside of the integration tests. |
Summary
This PR includes two commits:
Fixes #1667