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

Add tests for freestyle dynamic #171

Merged

Conversation

danwenzel
Copy link
Collaborator

@danwenzel danwenzel commented Apr 9, 2018

Adds acceptance tests and a property assertion/test for freestyle dynamic

@danwenzel danwenzel force-pushed the add-tests-for-freestyle-dynamic branch from d6ddf51 to 577ab96 Compare April 9, 2018 02:11
@lukemelia
Copy link
Collaborator

@danwenzel I'm excited that you've followed up with tests for the freestyle-dynamic work. The following is just my opinion: I don't think page objects are a good fit for an opensource project. They essentially define a new DSL that you need to learn in order to understand the tests, which you can make a case for on a team that is living with an test suite for years, but is harder when you want new contributors to be able to pop into an open source repo and help. I would prefer to see freestyle adopt a qunit.dom-style approach to assertions. Again, just my $.02.

@danwenzel
Copy link
Collaborator Author

Good thoughts, @lukemelia . I was following the pattern of the existing acceptance tests, so perhaps @chrislopresto has some thoughts. If we're going to make a change, it's probably better to do it throughout the test suite to keep things consistent .

@chrislopresto
Copy link
Owner

Awesome. Thanks, @danwenzel!

@lukemelia It may indeed be nice to refactor the entire test suite away from page objects. I'd be happy to merge this PR as is to gain the test coverage in the meantime, though. Thoughts?

@danwenzel
Copy link
Collaborator Author

@lukemelia - What do you think? Ok to merge this, and then separately refactor tests later?

@danwenzel
Copy link
Collaborator Author

@chrislopresto - Luke seems to be unresponsive :) Shall we just merge this?

@lukemelia
Copy link
Collaborator

Whoops, sorry. I'm +1. Don't let the perfect be the enemy of the good!

@chrislopresto chrislopresto merged commit a330edb into chrislopresto:master May 21, 2018
@chrislopresto
Copy link
Owner

Thanks @danwenzel!

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.

3 participants