Skip to content

tournament: Start with simpler tests / remove invald input test. #773

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 7, 2017
Merged

tournament: Start with simpler tests / remove invald input test. #773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
333d19d
Remove requirement to handle invalid input.
Insti May 5, 2017
f2042f1
Add more, simpler, initial tests.
Insti May 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 96 additions & 21 deletions exercises/tournament/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"exercise": "tournament",
"version": "1.1.0",
"version": "1.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

I might have missed it, but shouldn't this be 1.2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the invalid input requirement was '1.2.0' adding the new initial tests made it '1.3.0', since '1.2.0' was not a separate PR and has never been released I can see an argument that we should stay on '1.2.0'.

I have a slight preference for '1.3.0' since it requires no work to change.

But if you think it should be '1.2.0' I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. I don't really have a problem with using 1.3.0, I just didn't understand. It's fine to leave it as is

"comments": [
"The inputs and outputs are represented as arrays of strings to",
"improve readability in this JSON file.",
Expand All @@ -10,6 +10,101 @@
"a single string."
],
"cases": [
{
"description": "just the header if no input",
"property": "tally",
"input": [],
"expected": [ "Team | MP | W | D | L | P" ]
},
{
"description": "a win is three points, a loss is zero points",
"property": "tally",
"input": [
"Allegoric Alaskans;Blithering Badgers;win"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 1 | 1 | 0 | 0 | 3",
"Blithering Badgers | 1 | 0 | 0 | 1 | 0"
]
},
{
"description": "a win can also be expressed as a loss",
"property": "tally",
"input": [
"Blithering Badgers;Allegoric Alaskans;loss"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 1 | 1 | 0 | 0 | 3",
"Blithering Badgers | 1 | 0 | 0 | 1 | 0"
]
},
{
"description": "a different team can win",
"property": "tally",
"input": [
"Blithering Badgers;Allegoric Alaskans;win"
],
"expected": [
"Team | MP | W | D | L | P",
"Blithering Badgers | 1 | 1 | 0 | 0 | 3",
"Allegoric Alaskans | 1 | 0 | 0 | 1 | 0"
]
},
{
"description": "a draw is one point each",
"property": "tally",
"input": [
"Allegoric Alaskans;Blithering Badgers;draw"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 1 | 0 | 1 | 0 | 1",
"Blithering Badgers | 1 | 0 | 1 | 0 | 1"
]
},
{
"description": "There can be more than one match",
"property": "tally",
"input": [
"Allegoric Alaskans;Blithering Badgers;win",
"Allegoric Alaskans;Blithering Badgers;win"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 2 | 2 | 0 | 0 | 6",
"Blithering Badgers | 2 | 0 | 0 | 2 | 0"
]
},
{
"description": "There can be more than one winner",
"property": "tally",
"input": [
"Allegoric Alaskans;Blithering Badgers;loss",
"Allegoric Alaskans;Blithering Badgers;win"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 2 | 1 | 0 | 1 | 3",
"Blithering Badgers | 2 | 1 | 0 | 1 | 3"
]
},
{
"description": "There can be more than two teams",
"property": "tally",
"input": [
"Allegoric Alaskans;Blithering Badgers;win",
"Blithering Badgers;Courageous Californians;win",
"Courageous Californians;Allegoric Alaskans;loss"
],
"expected": [
"Team | MP | W | D | L | P",
"Allegoric Alaskans | 2 | 2 | 0 | 0 | 6",
"Blithering Badgers | 2 | 1 | 0 | 1 | 3",
"Courageous Californians | 2 | 0 | 0 | 2 | 0"
]
},
{
"description": "typical input",
"property": "tally",
Expand Down Expand Up @@ -64,26 +159,6 @@
"Blithering Badgers | 3 | 0 | 1 | 2 | 1",
"Devastating Donkeys | 3 | 0 | 1 | 2 | 1"
]
},
{
"comments": [
"Invalid input lines in an otherwise-valid game",
"still results in valid output."
],
"description": "mostly invalid lines",
"property": "tally",
"input": [
"",
"Allegoric Alaskans@Blithering Badgers;draw",
"Blithering Badgers;Devastating Donkeys;loss",
"Devastating Donkeys;Courageous Californians;win;5",
"Courageous Californians;Allegoric Alaskans;los"
],
"expected": [
"Team | MP | W | D | L | P",
"Devastating Donkeys | 1 | 1 | 0 | 0 | 3",
"Blithering Badgers | 1 | 0 | 0 | 1 | 0"
]
}
]
}
5 changes: 0 additions & 5 deletions exercises/tournament/description.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,3 @@ Devastating Donkeys;Courageous Californians;draw
```

Means that the Devastating Donkeys and Courageous Californians tied.

Your program should only process input lines that follow this format.
Copy link
Member

Choose a reason for hiding this comment

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

This PR comment is inspired by the fact that one of the questions asked in exercism/rust#122 is "should I expect invalid lines?" despite the fact that invalid lines were in fact included in the test suite at the time (so the answer to the question was specified in the test suite)

The alternative to removing completely is a sentence saying "You need only process input lines that follow this format, as invalid lines will not be tested."

I very weakly support the addition of such a sentence, because:

One might consider this a useful addition if students will ask this question and it is useful to get an answer without looking at the test suite. Note that it is easier to figure out "there is an invalid input" (you only need to look for any one test case that has it) than "there are no invalid inputs" (you need to look through all test cases and confirm that everything is valid).

Wouldn't it be the case that forcing the student to look at every test case to answer that question is going against the one-test-at-a-time metholodogy? Even if we pretended this is a scenario where I am writing the tests from scratch (instead of having them be provided but not run, as various tracks do), that I would want to know whether I would have to deal with invalid tests ahead of time?

Side notes:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @petertseng , good points.

"should I expect invalid lines?"

Isn't "invalid input will not be tested" up to the tracks to decide, since they are free to add or remove tests.

Wouldn't it be the case that forcing the student to look at every test case to answer that question is going against the one-test-at-a-time metholodogy?

Why would they be writing code to handle it without a failing test first? Once they get to the end they might notice that they have not had to handle invalid input.

(I'm not against adding something about test input validity to the description.)

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be the case that forcing the student to look at every test case to answer that question is going against the one-test-at-a-time metholodogy?

Why would they be writing code to handle it without a failing test first?

I don't think someone doing test-first will write that code without a test. But I don't think this affects whether I would want this question answered (whether I want the sentence to appear in the README) before seeing/writing a test.

If I pretend that I'm writing tests from scratch, I imagine I am aware of some set of requirements before having written any test and they inform the tests that I will write. I would look to the README for those requirements.

The general question would be stated as "What is the role of an exercise's README? To what extent need it describe requirements and non-requirements?" I would say that exercism/go#189 affects how I think about this question (steers me to ask more of a README).

Maybe nobody will worry about this. After all, the proposed sentence states a non-requirement. The consequence of omission is that maybe people code too defensively. That's less severe than the consequence of omitting a requirement.

It also does not really affect me since I generally do look at all the tests beforehand.

"should I expect invalid lines?"

Isn't "invalid input will not be tested" up to the tracks to decide, since they are free to add or remove tests.

Oh! So benefit of leaving the sentence off is to give tracks the option. Okay.

I hope we don't run into a situation where all tracks that don't take the option have to each add a HINTS.md saying "we won't test invalid inputs" if we leave the option open and students express a preference for answering the question upfront in the README. Of course, we don't need to think about that until it becomes a problem.

As I hope is indicated by my prior approval, it does not matter to me whether the proposed sentence is added or not, but I'm happy to continue discussing if it inspires anyone else to have a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference is to not add anything to the readme about what is tested.

I'd like to try the new readme that doesn't mention test input validity at all for a while and If it proves to be an issue in the future, someone can submit a PR to add the relevant language.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @Insti: omitting that line about validity seems to make the most sense to me.

All other lines should be ignored:
If an input contains both valid and invalid input lines,
output a table that contains just the results from the valid lines.