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

Refactor rspec tests #17

Merged
merged 1 commit into from
Oct 15, 2015
Merged

Refactor rspec tests #17

merged 1 commit into from
Oct 15, 2015

Conversation

onnimonni
Copy link
Contributor

This contains Refactoring of rspec tests.

I added all wordpress related functions into their own static module WP which can be found in tests/rspec/lib/wp.rb

This makes the rspec tests so much more readable.

@onnimonni
Copy link
Contributor Author

@anttiviljami if travis tests passes this should be good to go :).

@ottok
Copy link
Contributor

ottok commented Oct 9, 2015

This pull request contains in fact two issues: using .local for domain names and refactored rspec tests.

About rspec refactoring: So now test.rb calls for config.rb which includes a call for wp.rb? So every time config.rb is called in any test, it wp.rb tests will run again and again?

In bus.fi project we have a script that runs all tests that are in patch tests/rspec/*. With this architecture it is easy to write tests for projects without overlapping the upstream namespace, so it is possible to update to new versions of seravo/wordpress.

Actually that script is already in this project (https://github.com/Seravo/wordpress/blob/master/scripts/run-tests) but Travis-CI call test.rb directly instead of running the run-tests.sh script.

The README could say more than just that 'test.rb' is an example. It could for example say, that if a user wants to make their own tests, all they need to do is copy test.rb into a new filename and then write their own tests there. Test.rb could stay in place as it contains good basic test.

What happens to existing tests? Will this change break compatibility with existing projects that have already been developed with seravo/wordpress as a starting point? Hmm.. looking at current bus.fi sources if does not contain any calls to config.rb, only 'lib/matchers.rb' so it is probably not compatible with this project template anyway anymore.

end

domains << config['name']+".seravo.dev" #test https-domain-alias locally
domains << config['name']+".seravo.local" # test https-domain-alias locally
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this ? shouldn't it be *.wp-palvelu.local anyway? usually you just configure the domains you need to use for this site in the config.yml, for which the example file already contains this entry. See: https://github.com/Seravo/wordpress/blob/master/config-sample.yml#L13

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved that for other branch, but we need to have HTTPS_DOMAIN_ALIAS defined so that users can test https domain alias in vagrant. Is this what you're asking for?

@onnimonni
Copy link
Contributor Author

@otto: Sorry I accidentally added two features simultaneously. I fixed some parts out of this PR and moved local network features to this PR: #18

I don't believe we can support automatic compatibility with this upstream in child projects. It's up to those projects whether they wan't to use new features or not? This is just like they do it in our counterpart: https://github.com/roots/bedrock

If you want to have multiple test files you should only add lib/config.rb once for better performance.

onnimonni added a commit that referenced this pull request Oct 15, 2015
@onnimonni onnimonni merged commit b3aab7d into master Oct 15, 2015
@ypcs ypcs deleted the refactor-rspec-tests branch July 19, 2023 08:22
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.

None yet

3 participants