simple-linked-list: sync missing test cases and add peek/toList#3146
simple-linked-list: sync missing test cases and add peek/toList#3146AdityaPudasaini wants to merge 2 commits into
Conversation
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
|
These changes do affect test outcomes as they add 29 missing test cases to sync with the latest problem specifications. Happy to add [no important files changed] to the commit message if the maintainers prefer, but I believe re-running student tests is appropriate here. |
kahgoh
left a comment
kahgoh
left a comment
There was a problem hiding this comment.
Hi @AdityaPudasaini, thanks for raising the PR to update the tests.
These changes do affect test outcomes as they add 29 missing test cases to sync with the latest problem specifications.
Actually, some of the tests have already been written - the tests.toml just haven't been synced so configlet couldn't see them. As a result I noticed some of the tests have now been duplicated (this is mentioned in one of comments below).
Happy to add [no important files changed] to the commit message if the maintainers prefer, but I believe re-running student tests is appropriate here.
No need to add this comment - we'll add it at merge time if we think it is appropriate. But you are also right - adding new tests and new methods is going to break all the existing solutions.
| @DisplayName("count -> Empty list has length of zero") | ||
| public void countEmptyListHasLengthOfZero() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(); | ||
| assertThat(list.size()).isEqualTo(0); |
There was a problem hiding this comment.
This test is the same as aNewTestIsEmpty. In fact, I noticed there are more tests that have also been duplicated (e.g. popFromEmptyListIsAnError is a copy of popOnEmptyListWillThrow). Could you please remove the duplicates? I suspect you'll be able to remove the "old" version of the tests.
| List<T> toList() { | ||
| throw new UnsupportedOperationException("Please implement the SimpleLinkedList.toList() method."); | ||
| } | ||
| T[] asArray(Class<T> clazz) { | ||
| throw new UnsupportedOperationException("Please implement the SimpleLinkedList.asArray() method."); | ||
| } |
There was a problem hiding this comment.
I think it is a bit strange to have both these methods for this exercise. I think we should choose one to go with and remove the tests for the other. If we go with asArray then the toList tests can use asArray instead.
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList LIFO -> Empty linked list to list is empty") | ||
| public void toListLifoEmptyLinkedListToListIsEmpty() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(); | ||
| assertThat(list.toList()).isEmpty(); | ||
| } | ||
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList LIFO -> To list with multiple values") | ||
| public void toListLifoToListWithMultipleValues() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(new Integer[]{1, 2, 3}); | ||
| assertThat(list.toList()).containsExactly(1, 2, 3); | ||
| } | ||
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList LIFO -> To list after a pop") | ||
| public void toListLifoToListAfterAPop() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(); | ||
| list.push(1); | ||
| list.push(2); | ||
| list.push(3); | ||
| assertThat(list.pop()).isEqualTo(3); | ||
| list.push(4); | ||
| assertThat(list.toList()).containsExactly(4, 2, 1); | ||
| } | ||
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList FIFO -> Empty linked list to list is empty") | ||
| public void toListFifoEmptyLinkedListToListIsEmpty() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(); | ||
| assertThat(list.toList()).isEmpty(); | ||
| } | ||
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList FIFO -> To list with multiple values") | ||
| public void toListFifoToListWithMultipleValues() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(new Integer[]{1, 2, 3}); | ||
| assertThat(list.toList()).containsExactly(1, 2, 3); | ||
| } | ||
|
|
||
| @Disabled("Remove to run test") | ||
| @Test | ||
| @DisplayName("toList FIFO -> To list after a pop") | ||
| public void toListFifoToListAfterAPop() { | ||
| SimpleLinkedList<Integer> list = new SimpleLinkedList<>(); | ||
| list.push(1); | ||
| list.push(2); | ||
| list.push(3); | ||
| assertThat(list.pop()).isEqualTo(3); | ||
| list.push(4); | ||
| assertThat(list.toList()).containsExactly(4, 2, 1); | ||
| } |
There was a problem hiding this comment.
It doesn't really make sense to have both FIFO and LIFO tests. It should have just the FIFO set or the LIFO set. This is mentioned in the comments at the top of the canonical-data 👇🏻:
"The linked list implements a LIFO. A pop retrieves the last-in value. ",
"As such, the toList operation can be thought as being build using pops, ",
"which means list([1, 2, 3]).toList() would return [3, 2, 1]. ",
"Conversely, the toList is more intuitive to some if it returns data matching how the list was build, ",
"so list([1, 2, 3]).toList() would return [1, 2, 3].",
"",
"To preserve both options, this data has two test groups: toList LIFO and toList FIFO.",
"The tests in these groups are mutually exclusive.",
Also, the expectations don't match the canonical data in some of your tests either. For example, the expectation for toListLifoToListWithMultipleValues should be [3, 2, 1], not [1, 2, 3], and for toListFifoToListAfterAPop, it should be [1, 2, 4], not [4, 2, 1].
Anyway, to fix this:
- Decide whether to use the LIFO set or FIFO set. I'd prefer using the set that aligns best with the existing
asArraymethod. I think that is the LIFO set. - In
tests.tomlmark the removed tests as skipped. To do this, you can addinclude = false. See line 29 in the one for hamming for an example.
There was a problem hiding this comment.
Thanks for the detailed review @kahgoh! I'll remove the duplicate tests, go with the LIFO set, mark the FIFO tests as include = false in tests.toml, and fix the expectations. I'll push the changes shortly.
There was a problem hiding this comment.
Hi @kahgoh, I've addressed all the feedback:
Removed duplicate tests (old aNewListIsEmpty, popOnEmptyListWillThrow, etc.)
Kept only the LIFO set and marked FIFO tests as include = false in tests.toml
Fixed the toList expectations to match canonical data
All 27 tests pass locally
2 participants
Pull Request
Syncs the simple-linked-list exercise with the latest problem specifications.
Changes:
configlet syncpeek()method to stub and reference solutiontoList()method to stub and reference solution.meta/tests.tomlwith all new test UUIDsAll 34 tests pass locally.
Fixes #2959
Reviewer Resources:
Track Policies