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

fixing tests and upgrading to latest rpsec #36

Closed
wants to merge 1 commit into from

Conversation

tphoney
Copy link
Contributor

@tphoney tphoney commented Apr 17, 2015

No description provided.

travis, allow failures due to logrotate
@tphoney
Copy link
Contributor Author

tphoney commented Apr 18, 2015

@jhoblitt Travis tests were failing due to future parser on the logrotate module being used. (rodjek/puppet-logrotate#46)
I have updated to a later version of rspec.
Acceptance tests were failing, added "nocheckcertificate => true" to fix the issue.

@jhoblitt
Copy link
Owner

@tphoney What platform were the acceptance tests failing on? I have not seen the certificate error that folks have reported and don't really want to disable certificate checking the acceptance tests.

In rspec3, is_expected.to is equivalent to should, which is not deprecated in the implicit receiver form. I prefer should as it's few characters both to type and read.

@tphoney
Copy link
Contributor Author

tphoney commented Apr 22, 2015

The tests were failing against the default node. This was a fresh checkout with the latest gems.

I was having a play with http://yujinakayama.me/transpec/ the changes checked in. Where the recommendations it suggested.

@jhoblitt Thanks for having a look at this.

@jhoblitt
Copy link
Owner

I wasn't aware of transpec -- I should definitely look into it. The docs suggest that the "oneliner" conversion can be disabled. https://github.com/yujinakayama/transpec#conversions-enabled-by-default

I will try to reproduce the certificate validation error.

@jhoblitt
Copy link
Owner

I've reproduced the certificate error with the default centos 6.4 nodeset. It is not present with centos 7.0 -- I'm not yet sure if this is due to different versions of wget, changes to the google certificate, or the installed CA set.

@tphoney
Copy link
Contributor Author

tphoney commented Apr 22, 2015

agreed, i just saw it with the default nodeset. sorry if i am just creating churn for you.

@jhoblitt
Copy link
Owner

@tphoney No need to apologize at all! Any contribution is greatly welcomed.

The acceptance nodesets have needed to be updated for awhile. I've been trying to hold off on a round test infrastructure updates across all of my modules until a stable release of rspec-puppet/puppetlabs_spec_helper is compatible with puppet 4.0.0.

I've pushed up a branch with updated gem versions and nodeset files. https://github.com/jhoblitt/puppet-selenium/tree/maint/test_infra

The centos-6.6 set in this branch passes for me without disabling the certificate check. Could you give it a try and let me know? I suspect, but have not tested, that the wget and/or ca-certificates packages in ~ el 6.4 dislike the SAN certificate currently being returned by https://selenium-release.storage.googleapis.com .

I'm not enthusiastic about disabling the cert check without some sort of digest verification of the jar file. Do you feel it would be reasonable to handle this as a docfix and add a note to the README about certificate validation errors?

@tphoney
Copy link
Contributor Author

tphoney commented Apr 22, 2015

That all seems totally sane, i will check out the code, and see if it all passes. Thanks again for your patience.

@jhoblitt jhoblitt mentioned this pull request Apr 22, 2015
@jhoblitt
Copy link
Owner

I've gone ahead and merged #37 . Would you be interested in drafting some words for the README about certificate validation errors or retrying the transpec update without modifying the implicit receiver form of should?

@tphoney
Copy link
Contributor Author

tphoney commented Apr 23, 2015

i was testing that :D, i will draft something for the readme. I will explain what the error is, and give your explanation of the error.

@tphoney
Copy link
Contributor Author

tphoney commented Apr 23, 2015

The tests ran through fine, i will look at the readme and rspec change and i will close this pr

Thanks again

@tphoney tphoney closed this Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants