updating tests diffie-hellman #2584

jagdish-15 · 2025-01-05T20:16:26Z

Pull Request

This PR syncs tests.toml with the problem-specifications repository and verifies the presence of new tests in the test file.

github-actions · 2025-01-05T20:16:36Z

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.

exercises/practice/diffie-hellman/.meta/tests.toml

Cool-Katt · 2025-01-06T09:29:39Z

I was going to make a comment about the tests in this exercise not being very consistent with the naming in tests.toml but upon closer inspection, I noticed that this exercise has been marked as deprecated in exercism/problem-specifications#2149 so I'm thinking I'll let it slide.
Thoughts @SleeplessByte ?

SleeplessByte · 2025-01-06T09:33:08Z

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.

Cool-Katt · 2025-01-06T09:38:09Z

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.

jagdish-15 · 2025-01-06T10:06:23Z

I’ve renamed one of the test sections to match the description in the tests.toml file. However, I couldn’t find the test corresponding to the "private key is random" label in the JavaScript file.

Also, @Cool-Katt and @SleeplessByte, could you let me know what you’ve agreed upon regarding marking the exercise as deprecated? Personally, I don’t think there’s any issue with keeping it as @Cool-Katt suggested, but I’m happy to proceed with whatever you all decide.

SleeplessByte · 2025-01-06T10:08:50Z

@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!

SleeplessByte · 2025-01-06T10:09:29Z

(we have extra tests that are not part of the official tests. That's fine!)

jagdish-15 · 2025-01-06T10:21:20Z

@SleeplessByte, what I meant was that the "private key" test from the toml file isn't present in the JS file. However, there are some extra tests in the JS file, and as you mentioned, having extra tests is fine since we have some unofficial tests. So, it's all good now!

Cool-Katt · 2025-01-06T11:25:38Z

exercises/practice/diffie-hellman/diffie-hellman.spec.js

@@ -14,7 +14,7 @@ describe('diffie-hellman', () => {
    }).toThrow();
  });

-  describe('input validation', () => {


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.

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!

best of luck on the exam!

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?

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

Sure, will get to it as soon as possible!

@jagdish-15 how is it going now? Still have time for this?

@SleeplessByte, I apologize for the delay—I didn’t intend to hold up the PR. I was occupied with organizing a college club event for CodePVG. I’ll get to work on this right away!

@SleeplessByte and @Cool-Katt,

I've made the necessary changes and implemented the getPrivateKey function along with the required tests. I ran format.mjs, lint.mjs, and test.mjs locally, and everything passed. However, the tests on GitHub are still failing. Could you help me identify the issue so I can fix it?

#2604

Not because of you. I'll figure that out.

Thanks for making the changes!

…ish-15/javascript into update-tests-diffie-hellman Merging with remote

Cool-Katt · 2025-02-11T08:51:08Z

@SleeplessByte so should we wait to sort out the CI or should we merge this?
I'm for a tentative merge, i think.

SleeplessByte · 2025-03-13T08:53:22Z

@Cool-Katt because CI is broken, it won't be deployed at all. I'll see if I can look into that next week. Ping me if not.

jagdish-15 · 2025-04-01T12:02:59Z

@Cool-Katt @SleeplessByte
Is it gonna take a while to be fixed?

jagdish-15 · 2025-06-02T07:17:19Z

@SleeplessByte , the issue has been taken care of right? Should we merge this now?

SleeplessByte · 2025-06-02T22:20:56Z

@SleeplessByte , the issue has been taken care of right? Should we merge this now?

#2637 needs to be merged, and then #2638 needs to be merged. The result needs to be merged into this / this needs to be rebased. That will make all the failing CI pass again, and THEN this is done! <3

Cool-Katt · 2025-06-03T07:06:45Z

@SleeplessByte Both prerequisite PRs have been merged o7

SleeplessByte · 2025-06-03T07:58:56Z

@jagdish-15 if you have a moment, can you rebase? If not we will do it next week.

Thanks again!

jagdish-15 · 2025-06-03T08:04:00Z

Sure, I'll rebase it by EOD! And now I can start working on updating the test of further exercises right? Since the CL issue has been fixed now.

SleeplessByte · 2025-06-03T10:33:00Z

Sure, I'll rebase it by EOD! And now I can start working on updating the test of further exercises right? Since the CL issue has been fixed now.

Yes! ❤️

…ish-15/javascript into update-tests-diffie-hellman Merging with origin

Cool-Katt · 2025-06-03T13:22:07Z

// diffie-hellman › private key is greater than 1 and less than p

    expect(received).toBeLessThan(expected)

    Expected: < 23
    Received:   23

That's randomness for you. Of course the very first time the tests run it randomly gets a value that makes the test fail.

jagdish-15 · 2025-06-03T13:27:53Z

That's randomness for you. Of course the very first time the tests run it randomly gets a value that makes the test fail.

Makes sense! Is it happening every time?
I mean, when I ran the tests locally for the first time after the failed tests here, it failed on the local system too, but the second time it passed. Strange, isn't it?

Cool-Katt · 2025-06-03T13:31:13Z

Makes sense! Is it happening every time? I mean, when I ran the tests locally for the first time after the failed tests here, it failed on the local system too, but the second time it passed. Strange.

Well. I don't think it should do that but I also think technically it's fine, the tests are the more important part of the PR and the proof solution is mostly to satisfy the CI gods. Generally, anything that has to do with randomness is hard to get consistency out of.

exercises/practice/diffie-hellman/.meta/proof.ci.js

jagdish-15 · 2025-06-05T17:12:03Z

@SleeplessByte
I tested the code after your suggestion by temporarily replacing the random part with 0.9999 to simulate Math.random() returning the maximum value. The issue where the random key wasn't strictly less than p no longer occurs.

After the test, I reverted it back to use the actual random expression. Everything seems to be working fine now. Could you please review the final expression for generating the random key? If it looks good to you, I think we’re ready to merge!

SleeplessByte · 2025-06-05T17:26:30Z

Yep. This should work.

Resolved

updating tests diffie-hellman

c1ab732

github-actions bot closed this Jan 5, 2025

Cool-Katt reopened this Jan 6, 2025

Cool-Katt requested changes Jan 6, 2025

View reviewed changes

exercises/practice/diffie-hellman/.meta/tests.toml Show resolved Hide resolved

jagdish-15 added 2 commits January 6, 2025 15:27

Updating names in test file to match those in test.toml

ac1da54

fixing formatting issue

d71c63d

Cool-Katt reviewed Jan 6, 2025

View reviewed changes

jagdish-15 added 2 commits February 4, 2025 21:43

Adding getPrivateKey implementation and tests

dd07216

Merge branch 'update-tests-diffie-hellman' of https://github.com/jagd…

7bc0075

…ish-15/javascript into update-tests-diffie-hellman Merging with remote

Cool-Katt approved these changes Feb 11, 2025

View reviewed changes

jagdish-15 added 3 commits June 3, 2025 18:41

updating tests diffie-hellman

489f9eb

Adding getPrivateKey implementation and tests

177987c

Updating names in test file to match those in test.toml

b33945b

jagdish-15 added 3 commits June 3, 2025 18:41

fixing formatting issue

efba153

Merge branch 'exercism:main' into update-tests-diffie-hellman

58c2812

Merge branch 'update-tests-diffie-hellman' of https://github.com/jagd…

6687c47

…ish-15/javascript into update-tests-diffie-hellman Merging with origin

SleeplessByte previously requested changes Jun 3, 2025

View reviewed changes

exercises/practice/diffie-hellman/.meta/proof.ci.js Outdated Show resolved Hide resolved

Fixing proof solution

2b86e9f

SleeplessByte approved these changes Jun 6, 2025

View reviewed changes

SleeplessByte merged commit 3c3f39a into exercism:main Jun 6, 2025
6 checks passed

Uh oh!

updating tests diffie-hellman #2584

updating tests diffie-hellman #2584

Uh oh!

Conversation

jagdish-15 commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Uh oh!

github-actions bot commented Jan 5, 2025

Uh oh!

Uh oh!

Cool-Katt commented Jan 6, 2025

Uh oh!

SleeplessByte commented Jan 6, 2025

Uh oh!

Cool-Katt commented Jan 6, 2025

Uh oh!

jagdish-15 commented Jan 6, 2025

Uh oh!

SleeplessByte commented Jan 6, 2025

Uh oh!

SleeplessByte commented Jan 6, 2025

Uh oh!

jagdish-15 commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cool-Katt commented Feb 11, 2025

Uh oh!

SleeplessByte commented Mar 13, 2025

Uh oh!

jagdish-15 commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jagdish-15 commented Jun 2, 2025

Uh oh!

SleeplessByte commented Jun 2, 2025

Uh oh!

Cool-Katt commented Jun 3, 2025

Uh oh!

SleeplessByte commented Jun 3, 2025

Uh oh!

jagdish-15 commented Jun 3, 2025

Uh oh!

SleeplessByte commented Jun 3, 2025

Uh oh!

Cool-Katt commented Jun 3, 2025

Uh oh!

jagdish-15 commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cool-Katt commented Jun 3, 2025

Uh oh!

Uh oh!

jagdish-15 commented Jun 5, 2025

Uh oh!

SleeplessByte commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

jagdish-15 commented Jan 5, 2025 •

edited

Loading

jagdish-15 commented Apr 1, 2025 •

edited

Loading

jagdish-15 commented Jun 3, 2025 •

edited

Loading