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

Cleanup skipping #22

Merged
merged 6 commits into from
Sep 29, 2014
Merged

Cleanup skipping #22

merged 6 commits into from
Sep 29, 2014

Conversation

Mezzle
Copy link

@Mezzle Mezzle commented Sep 24, 2014

PHPUnit has in built test skipping, this should be used over @group skip,
as this means that testing commands can be ran from any working directory
rather than from the base php directory. Unsure as to whether exercism will
actually currently provide the phpunit.xml - but this means there's now no
longer a need to serve files to the root either

Martin Meredith added 3 commits September 24, 2014 10:58
PHPUnit has in built test skipping, this should be used over @group skip,
as this means that testing commands can be ran from any working directory
rather than from the base php directory.  Unsure as to whether exercism will
actually currently provide the phpunit.xml - but this means there's now no
longer a need to serve files to the root either
@Mezzle
Copy link
Author

Mezzle commented Sep 24, 2014

Having looked into this further, I've realised the reason for this - so that the tests can all be ran against the examples in the Makefile.

I've added a solution for this - but it feels "hacky".

Personally, I prefer the benefits of using markTestSkipped() as it allows the test files to be ran "correctly" from anywhere... but that's me.

If you're not happy with this change - it'll take but a moment to correct my other submissions to match the old style @group skipped :)

@kytrinyx
Copy link
Member

Thanks for weighing in on this. I'm not actually convinced that we need to have them skipped at all (which sure would make the tests easier on CI).

There's been a fairly meaty discussion about the pros and cons: exercism/exercism#856

@wilmoore What do you think?

@Mezzle
Copy link
Author

Mezzle commented Sep 24, 2014

I'm of the opinion that tests should only be skipped if they can't run on a specific system. If they're just failing, they either shouldn't be there, or they should fail.

Adding the skips seems like it's potentially teaching the wrong lesson regarding as to when "skipping tests" should be used?

@wilmoore
Copy link

There's been a fairly meaty discussion about the pros and cons: exercism/exercism#856
@wilmoore What do you think?

I would personally prefer no skips; however, I realize that there are benefits to having the skips for a certain level of developer. That being said, I suspect there may be other ways to solve that problem, so perhaps we can do away with the skips and address the other issues separately.

@kytrinyx
Copy link
Member

Yeah, the skips are helpful for people who are quite new to programming (though not at all helpful for people who are completely new to programming... that's a different problem).

I'm now leaning towards getting rid of the skips altogether, except maybe in Ruby, JavaScript, and perhaps Python (where we get a lot more beginners).

@wilmoore
Copy link

except maybe in Ruby, JavaScript, and perhaps Python

Or...perhaps we just provide better on-boarding across the board?

@kytrinyx
Copy link
Member

Or...perhaps we just provide better on-boarding across the board?

Working on it.

@wilmoore
Copy link

So, I think this PR would work if all of the skips got dropped.

@Mezzle
Copy link
Author

Mezzle commented Sep 25, 2014

Will do so tomorrow when I'm at a PC with dev tools on it

@Mezzle Mezzle closed this Sep 25, 2014
@Mezzle Mezzle changed the title Update to use ->markTestSkipped() Cleanup skipping Sep 26, 2014
@Mezzle Mezzle reopened this Sep 26, 2014
@kytrinyx
Copy link
Member

@Mezzle I'm seeing new commits. Is this done? The test suite doesn't look happy, but I haven't looked into what's causing that in case you're still working on it.

@Mezzle
Copy link
Author

Mezzle commented Sep 29, 2014

I pushed new commits, but didn't wait to check on travis!

I've re-added one skip - where the test code provides an example of the test (testing the gigasecond for your own birthday)

Assuming this passes, it should now be complete.

@kytrinyx
Copy link
Member

This looks good. Thank you!

kytrinyx added a commit that referenced this pull request Sep 29, 2014
@kytrinyx kytrinyx merged commit 3027694 into exercism:master Sep 29, 2014
@Mezzle Mezzle deleted the cleanup-skipping branch September 29, 2014 13:25
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