updating tests diffie-hellman #2584
Conversation
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
|
I was going to make a comment about the tests in this exercise not being very consistent with the naming in |
|
Eh. Right. I think because the change is not hard @jagdish-15 can you rename the test in the JavaScript file to match the description of the test in tests.toml? Can you also mark the exercise as deprecated in the config.json? @Cool-Katt good stuff. |
|
Do we need to deprecate it tho? In the PR that deprecates this it seems like people reached the conclusion that tracks that have it already implemented may chose to keep it if they feel like it. Personally, i don't think it's a particularly exciting exercise, but I also don't think it should be booted off, as it's not the worst in terms of complexity. |
|
I’ve renamed one of the test sections to match the description in the Also, @Cool-Katt and @SleeplessByte, could you let me know what you’ve agreed upon regarding marking the exercise as |
|
@Cool-Katt if you think we should keep it, we'll keep it. I don't have an opinion on this particular one. @jagdish-15 what did you mean with not being able to find "private key"? Do you mean you cannot find the test in JS or cannot find the test from JS in the .toml file? Sorry I have not checked the files yet! |
|
(we have extra tests that are not part of the official tests. That's fine!) |
|
@SleeplessByte, what I meant was that the "private key" test from the |
| @@ -14,7 +14,7 @@ describe('diffie-hellman', () => { | |||
| }).toThrow(); | |||
| }); | |||
|
|
|||
| describe('input validation', () => { | |||
There was a problem hiding this comment.
Since our tests include custom cases that are not part of the predefined ones, the structure of our test file in not standard either. As such, the name of this section of test cases is not incorrect, and shouldn't be changed.
However, it is missing two of the standard tests, private key is greater than 1 and less than p and private key is random (i think they would be placed under validation).
@jagdish-15 can you implement the two missing tests? You can look at other tracks for inspiration, for example something similar to the Java or C# tracks' implementations.
There was a problem hiding this comment.
Sure! I'll make the changes by checking out other tracks for inspiration. It'll take a bit of time since I need to prepare for a college exam tomorrow, but I'll get to it once the exam is over. Thanks for your understanding!
There was a problem hiding this comment.
best of luck on the exam!
There was a problem hiding this comment.
Thank you so much for the kind words, @Cool-Katt! I really appreciate it!
Regarding the tests, I realized while implementing them that they check the getPrivateKey method, which isn't included in either the student starter code or the proof solution. After reviewing other tracks, I saw that this method typically returns a private key, randomly chosen between 1 and p, exclusive (i.e., strictly between 1 and p). Since the tests are designed to validate this, it seems we should have such a method in place.
Would you like me to add the getPrivateKey method to both the proof solution and the starter code for students?
There was a problem hiding this comment.
Yeah, that would probably explain why the tests for validating the private key were missing 😅
You can also amend the stub and the ci proof
There was a problem hiding this comment.
Sure, will get to it as soon as possible!
Pull Request
This PR syncs
tests.tomlwith theproblem-specificationsrepository and verifies the presence of new tests in the test file.