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

Better Formatter tests #120

Merged
merged 18 commits into from Feb 6, 2017
Merged

Conversation

@bigfoot90
Copy link
Contributor

bigfoot90 commented Jan 6, 2017

Writing tests this way can be harden, but you can be sure that your feature/fix for an formatter type will not impact on any other formatter type.

WDYT?
If you think that can be useful I'll continue to write tests for the remaining tests cases.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 6, 2017

Codecov Report

Merging #120 into master will increase coverage by -0.14%.

@@             Coverage Diff              @@
##             master     #120      +/-   ##
============================================
- Coverage     99.86%   99.73%   -0.14%     
- Complexity        0     1607    +1607     
============================================
  Files            54       54              
  Lines          3785     3787       +2     
============================================
- Hits           3780     3777       -3     
- Misses            5       10       +5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59f39f9...96a904a. Read the comment docs.

@nijel

This comment has been minimized.

Copy link
Member

nijel commented Jan 7, 2017

I think this is good idea, just make sure that all things currently covered will stay covered.

@bigfoot90 bigfoot90 force-pushed the bigfoot90:better-formatter-tests branch 2 times, most recently from 8801355 to 264540e Jan 7, 2017
" 1 # Comment" . "\n",
'cli' =>
"\x1b[35mSELECT" . "\n" .
" \x1b[92m1 \x1b[37m# Comment\\x0A" . "\x1b[0m",

This comment has been minimized.

Copy link
@bigfoot90

bigfoot90 Jan 8, 2017

Author Contributor

Found first issue writing tests.
Why there is a \x0A in the comment?

This comment has been minimized.

Copy link
@bigfoot90

bigfoot90 Jan 8, 2017

Author Contributor

Fixing PR #123

@bigfoot90 bigfoot90 mentioned this pull request Jan 8, 2017
@bigfoot90 bigfoot90 force-pushed the bigfoot90:better-formatter-tests branch from b94eee2 to 5a328b7 Jan 8, 2017
@nijel nijel self-assigned this Jan 17, 2017
@nijel

This comment has been minimized.

Copy link
Member

nijel commented Jan 20, 2017

Any progress on this?

@bigfoot90

This comment has been minimized.

Copy link
Contributor Author

bigfoot90 commented Jan 20, 2017

Yes, I was waiting for #123 to rebase onto.

@bigfoot90 bigfoot90 force-pushed the bigfoot90:better-formatter-tests branch 2 times, most recently from 6af4ff3 to a370639 Jan 23, 2017
@bigfoot90 bigfoot90 force-pushed the bigfoot90:better-formatter-tests branch 3 times, most recently from dbebeb1 to bc06c1d Jan 26, 2017
@bigfoot90

This comment has been minimized.

Copy link
Contributor Author

bigfoot90 commented Jan 27, 2017

I'm ready for the review, yo can see that every test case in testFormat is covered by the new tests in testFormat_new.
Any comments or suggestions?

Actually only the codacy/pr tests are failing, this will be resolved when I remove the old tests in testFormat and replace testFormat_new with testFormat.

Copy link
Member

nijel left a comment

I think it's pretty much ready to be merged, just rename the method and remove the old tests.

* Previous Token.
*
* @var Token $prev

This comment has been minimized.

Copy link
@nijel

nijel Jan 30, 2017

Member

This is really not variable declaration, so the comment does not make much sense here...

if ($prev !== null) {
/** @var Token $prev */

This comment has been minimized.

Copy link
@bigfoot90

bigfoot90 Jan 30, 2017

Author Contributor

@nijel WDYT about this way?
I known that there is not a variable declaration, this way I say a bit different thig.
Here the annotation just says that there is a variable $prev of type Token.

This annotation is demanded by IDE, else it says that there is not any property value on null.

If it is not ok I can revert this one, no problem.

This comment has been minimized.

Copy link
@nijel

nijel Feb 2, 2017

Member

But $prev is declared before, I see no reason to have it annotated here...

This comment has been minimized.

Copy link
@bigfoot90

bigfoot90 Feb 2, 2017

Author Contributor

There is already a annotation on the declaration (on line 325), but PhpStorm is simply ignoring it.

@bigfoot90 bigfoot90 force-pushed the bigfoot90:better-formatter-tests branch from c699dfc to 9a120df Feb 1, 2017
@bigfoot90

This comment has been minimized.

Copy link
Contributor Author

bigfoot90 commented Feb 3, 2017

@nijel Removed @var anotation.
Are there other issues with this PR?
I can proceed to fix the method name testFormat_new and remove the old deprecated tests?

@nijel

This comment has been minimized.

Copy link
Member

nijel commented Feb 4, 2017

I think it's in good shape, please go ahead.

@nijel nijel merged commit 96a904a into phpmyadmin:master Feb 6, 2017
3 of 4 checks passed
3 of 4 checks passed
codecov/project 99.73% (-0.14%) compared to 59f39f9
Details
codacy/pr Good work! A positive pull request.
Details
codecov/patch Coverage not affected when comparing 59f39f9...96a904a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
nijel added a commit that referenced this pull request Feb 6, 2017
Issue #120

Signed-off-by: Michal Čihař <michal@cihar.com>
@nijel

This comment has been minimized.

Copy link
Member

nijel commented Feb 6, 2017

Thanks, I've merged it.

The coverage failure was caused by formatter handling differently BEGIN and begin as you've changed this. I've fixed this in 5fdfa5b and 1a11681 and adjusted tests accordingly in 65fb611.

@bigfoot90

This comment has been minimized.

Copy link
Contributor Author

bigfoot90 commented Feb 6, 2017

Thanks
Just to be clear. This way I'm testing that keywords are recognized and uppercased correctly.

@nijel

This comment has been minimized.

Copy link
Member

nijel commented Feb 7, 2017

They were uppercased properly, but the formatting didn't recognize because of being lowercase...

@bigfoot90 bigfoot90 deleted the bigfoot90:better-formatter-tests branch Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.