Revise Hamming tests with missing cases from specification #48
Conversation
exercises/hamming/lib/hamming.dart
Outdated
| // Put your code here | ||
| int compute(String a, String b) { | ||
| // Put your code here | ||
| } |
There was a problem hiding this comment.
This can be reverted. The users will know it needs to be implemented from the tests. Best let them decide the details like the types used.
jerold
force-pushed
the
40_43_hamming_updates
branch
from
44b5f1c
to
e99cd10
Compare
There was a problem hiding this comment.
@jerold This is a great job.
But I feel like I may have misspoke my direction.
I meant to say, add the following 6 cases to the previous test suite. If any of the 6 cases are duplicates, then don't add them.
The first case (empty strands) from the json is not implemented.
And the following cases:
non-unique character in first strand
non-unique character in second strand
same nucleotides in different positions
disallow first strand longer
disallow second strand longer
Additionally, there some of the tests ended with }, skip: true);. Leave those in place, since only the first test should not be skipped.
Also, it may be an issue on Travis CI's side, but the build of this PR has been going on for 1 hour and 42 min. So I'm going to restart it.
|
@jerold Just checking in, how do you feel about my feedback? |
|
@Stargator Sorry about the delay. I was out of town last weekend. I'm going to clean this up today. |
|
@jerold No worries. Thanks for remembering us 😸 |
|
@Stargator restored the previous set of tests and implemented the 6 you've asked for. Run the new tests against the example hamming implementation and all pass. Ready for re-review. |
|
@Stargator Sorry for the churn. The previous commit by itself may look a little confusing. I effectively undid the previous commit (where I'd reordered/renamed the tests to exactly match the canonical-data.json file). Looking at the diff for the whole pr currently should show 6 tests added:
Do you mean to say the bottom 5 of those should be skipped, or should all tests in the file, minus the top test be skipped? Are these tests brought online one by one somehow as the coder works on their implementation? I'm curious what the process is for these tests that required them to be skipped initially? Are these part of multi-part tutorial? |
|
@jerold The tests should be done one at a time. Generally, the top test is the first test (we don't skip the first test). Then all the tests that follow it will be appended with
Yes The process is that we are signalling to the user that the first problem for them is the only test that is failing, the first test. Then they solve that problem by passing the first test. Then they stop skipping the second test, see that it fails and work to resolve it. Then repeat the cycle until all the tests pass. |
| final res = hamming.compute("AGG", "AGA"); | ||
|
|
||
| expect(res, equals(1)); | ||
| }, skip: true); |
There was a problem hiding this comment.
I can't see a reasonable difference between the two tests.
For "non-unique character in first strand", if the first strand was "AAG" and the second strand was "AAA", then I would understand the problem.
Then reverse it for "non-unique character in second strand".
|
@Stargator give this a look. I've translated the full test suite described in canonical-data.json to dart tests. Each test has a counterpart. Same descriptions, cases, expectations. The diff is a little ugly, but take a look at the current raw test file next to canonical, and I think you'll like it. Otherwise I can revert, but in that case I would think a change really should be made to canonical first, and the tests then made to match here in hamming_test.dart. |
|
@jerold Understood. But do understand, we don't have to follow the canonical-data 1 to 1. So I opened a PR in the problem-specifications exercism/problem-specifications#953, feel free to comment. Outside of that, the files need to be formatted they are failing the CI Build. |
Stargator
changed the title
Issues
#40
Updating tests for Dart hamming distance example.