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

Fix for issue #552 including acceptance tests #556

Merged
merged 4 commits into from
Oct 16, 2016

Conversation

jskarpe
Copy link
Contributor

@jskarpe jskarpe commented Sep 23, 2016

Adding Docker nodesets for
Debian 7/8
Ubuntu 12/14/16
Ommiting CentOS 6/7 due to bug

@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 23, 2016

Seems Travis needs to retry on Debian

@jskarpe jskarpe closed this Sep 23, 2016
@jskarpe jskarpe reopened this Sep 23, 2016
@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 26, 2016

Fixes #552 with acceptance tests to prove it

@jskarpe jskarpe changed the title Add Docker nodesets and enable in TravisCI Fix for issue #552 with acceptance tests Sep 27, 2016
@jskarpe jskarpe changed the title Fix for issue #552 with acceptance tests Fix for issue #552 including acceptance tests Sep 27, 2016
@daenney
Copy link
Member

daenney commented Sep 27, 2016

This is way way way too many commits. Please do some work to rewrite history in much more processable and contained sets of commits.

Also, the changes to .travis.yml might get module synced away.

@daenney daenney added enhancement New feature or request needs-squash needs-work not ready to merge just yet labels Sep 27, 2016
@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 27, 2016

I did a rebase from master. Apparently incorrectly

@daenney
Copy link
Member

daenney commented Sep 27, 2016

I'm not sure the Travis+Docker changes is something we want to do right now. This is going to dramatically slow down tests, it's adding 9 tests to the matrix which almost doubles the matrix size.

@daenney daenney added needs-feedback Further information is requested and removed needs-squash needs-work not ready to merge just yet labels Sep 27, 2016
@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 27, 2016

The module was unusable on CentOS 6 and 7 for instance. The added tests would ensure that doesn't happen again

@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 27, 2016

Not sure I agree with "dramatically" though? Spec test on Ruby 2.3.1 is already 4 minutes, which is longer than most of the Beaker tests. In my opinion the Beaker tests have more value than multiple ruby versions

@daenney
Copy link
Member

daenney commented Sep 27, 2016

It's at worst 9x~3m that get added to the test time when it's busy and things can't get scheduled in parallel, That's almost 20 minutes extra wait time, in practice it's probably around 10m.

I agree that we could/should remove some of the Rubies though in general. If a module doesn't have native code in it it's not that useful.

@jskarpe
Copy link
Contributor Author

jskarpe commented Sep 27, 2016

In my opinion it's still worth it :-)

@bastelfreak
Copy link
Member

IMO we should wait with the acceptance test until voxpupuli/modulesync_config#230 is resolved. Maybe it is a good idea to add the tests from this PR in a new one?

@daenney
Copy link
Member

daenney commented Sep 29, 2016

The adding docker CI and revert should be squashed to just eliminate them from history.

@jyaworski jyaworski merged commit 4c0ef88 into voxpupuli:master Oct 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-feedback Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants