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 the lexer #18

Merged
merged 1 commit into from Sep 10, 2015
Merged

Add tests for the lexer #18

merged 1 commit into from Sep 10, 2015

Conversation

juanpaucar
Copy link
Contributor

@jsl @mcelicalderon Could you take a look, please? We used several of Mario's tests for the lexer

@@ -0,0 +1,145 @@
require_relative "../test_helper"

describe SimpleTemplates::Lexer do
Copy link
Member

Choose a reason for hiding this comment

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

Did you make sure that these fail in the right place, by writing the test, and then changing the code to make the test red?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We tested the new additions and check the edges cases for the VALID_PLACEHOLDER in the lexer

@jsl
Copy link
Member

jsl commented Sep 9, 2015

This is looking pretty good. Can you rebase this on top of my other branch (not sure if you've done that already).

@juanpaucar juanpaucar changed the title Add tests for the lexer [WIP] Add tests for the lexer Sep 9, 2015
@juanpaucar juanpaucar changed the title [WIP] Add tests for the lexer Add tests for the lexer Sep 9, 2015
@juanpaucar
Copy link
Contributor Author

@jsl Done. Rebased on top of your branch

@mpapis
Copy link
Contributor

mpapis commented Sep 10, 2015

LGTM, can you rebase on top of master (#17 is merged)

as we are improving tests here can you also run it with Mutant => http://solnic.eu/2013/01/23/mutation-testing-with-mutant.html (it could be separate PR if Mutant finds a lot of problems)

@mpapis
Copy link
Contributor

mpapis commented Sep 10, 2015

also please update #4

@juanpaucar
Copy link
Contributor Author

@mpapis It's a great idea to use mutation tests for what I've seen. But currently we are using minitest for the test suite, and mutant currently only supports rspec. So we would have to move to rspec? any thoughts on this?

@jsl
Copy link
Member

jsl commented Sep 10, 2015

@juanpaucar I'd prefer to avoid switching to rspec, since it's a significantly heavier dependency. At any rate, that could be in another PR. I'll open an issue to investigate mutation testing or other property-based testing, since that could be useful for the library.

@jsl jsl merged commit 2c02fea into master Sep 10, 2015
@juanpaucar juanpaucar deleted the add_tests_for_lexer branch September 11, 2015 13:41
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