remove last test from smallest common multiple challenge?

[moT01]

I have seen a significant number of posts on the forum about this challenge - and peoples code not passing because it is not efficient enough. It's seems like it's always that they pass all the tests but the last one.

I don't think this challenge should be about making your code more efficient.

I'm wondering if we should just remove that last test quick.

[RandellDawson]

I could be wrong, but do these same inefficient solutions fail on the current curriculum on master? I was thinking there was a change which increased the amount of time allowed for the tests to execute.

[ojeytonwilliams]

It's an inherently difficult thing to reproduce, since it depends on how fast your machine is. Also, I'm not sure what's in production right now - if it's the production-current branch then the timeout is 100ms which hasn't changed since April 2018 if not earlier than that.

Anyway, I agree with @moT01 that we should take the last test out - I don't think people should be punished for having a slow machine.

[RandellDawson]

Also, the 2nd to last test sometimes fails for some people. Why not increase the timeout to say 200ms or 300ms? I personally, would rather see this challenge moved to the JavaScript Algorithms and Data Structures Projects section and not drop any of the test cases. Why? Because I think it is important for users to learn how to use more efficient algorithms instead of watering down the challenges.

[moT01]

I don't mind increasing the timeout - but even at 200-300ms, I'm thinking a number peoples algorithms still wouldn't pass the last test. And if we want people to make more efficient code then that should be part of the focus of the challenge. It is not really taught anywhere how to make the most efficient code or test performance. And the wording of the challenge doesn't say anything about there being a time restriction and that the code needs to be efficient.

I wouldn't mind seeing this moved to the projects and the challenge changed to focus on efficiency, but I'm not sure we should do it with these lessons going away in the future. I think a quick and easy fix here is best. So I say we just remove the last test, and possibly increase the timeout to 200ms or 300ms - my opinion anyway.

[ojeytonwilliams]

I'm actually a bit opposed to increasing the timeout, because I don't think there's a solid reason to have tests that run for more than 100ms, let alone loops that run that long. I'd prefer it if we focused on writing tests that can be expected to pass or fail quickly.

One thing we could do is just copy this entire challenge into the projects section with maybe some even harder tests, with the instructions "this time you must use a fast algorithm" or similar.

I.e. either the tests should complete quickly or making them quick is explicitly part of the instructions.

[RandellDawson]

One thing we could do is just copy this entire challenge into the projects section with maybe some even harder tests, with the instructions "this time you must use a fast algorithm" or similar.

I vote for this. @scissorsneedfoodtoo What do you think about moving this challenge into the projects sections? I think the main issue is this makes it a required challenge to get the certification. Currently, there are only 5 challenges required to get the certification.

[scissorsneedfoodtoo]

Sorry for the delay everyone.

I believe the timeout was increased earlier this year like @RandellDawson mentioned, though I can't find the PR at the moment. It's my understanding that we won't be adding more projects to any sections of the curriculum, and that all of the current required projects will remain the same once the new curriculum rolls out.

I'm thinking the best thing to do now is to remove or simplify the last test so that more people have a chance of completing the challenge, even if their solution isn't the most efficient. This challenge is already quite difficult for intermediate learners. That, or we could make it more clear in the instructions that the solution needs to be pretty efficient to pass all test cases.

[moT01]

My vote is still to just remove the last test.

[RandellDawson]

My vote is still to just remove the last test.

You would have to also remove the 2nd to last test, because people also end up with inefficient solutions which do not pass it either. That being said, instead of watering down the challenge, I say move it to the Algorithms sub section of the Coding Interview Prep section.

[moT01]

The second to last test failing is much more rare - but yes, it does happen. Removing the last test would probably work for 90+% of cases.

It is one of the more difficult algorithms - moving it to coding interview prep might not be a bad idea - but moving it alone wouldn't address the issue of the tests not passing.

[RandellDawson]

I could be wrong, my guess is that most of the Project Euler tests are going to fail, because those challenges require extremely efficient algorithms. If we start taking tests out because people don't have efficient algorithms, we may have to remove many of the Project Euler tests too. Where does it end.

I think if you move it to the Algorithms section of the Coding Interview Prep, then there is an expectation you must have an efficient algorithm. In an interview, they are not going to accept a brute force solution.

[ojeytonwilliams]

@RandellDawson that's definitely true about the Project Euler questions. Many of them are explicitly designed so that you need to solve them efficiently. I don't think it makes sense to relax that requirement.

As for this challenge, I think

1. Remove the last two tests
2. Move it and warn people they need to use a fast algorithm

are the best two options. If it stays it needs to be solvable in a couple of minutes and most people (I expect) will just come up with a brute force solution first, so that needs to pass. If it's moved to Coding Interview Prep then it's expected to be challenging, so there's no problem if brute force solutions fail. Actually, in that case, it might be better to add a even tougher test so that brute force isn't an option.

[RandellDawson]

If two mods can agree on either of @ojeytonwilliams's option 1 or 2 above, I will be ok approving that same decision and merging the PR.

[ValeraS]

In master, JS challenges do not use a 100 ms limit per cycle. Only a 5-second limit per test is used for them.

[RandellDawson]

Only a 5-second limit per test is used for them.

So maybe this is a non-issue and we do not need to change anything. @moT01 Have you tested some of the solutions which users say do not pass on production using the master branch locally?

[moT01]

I was just doing this... I tested about a dozen algorithms from forums posts on master and two of them didn't pass the last test.

I didn't test them all on production, but the two I tested on production didn't pass the last two tests but passed them all on master.

[RandellDawson]

Would you mind sharing the two solutions which do not pass on master?

[moT01]

I'll have to go find them again - I had these two tabs still open that fail production and pass master... https://www.freecodecamp.org/forum/t/smallest-common-multiple-final-test-not-passing/281567 https://www.freecodecamp.org/forum/t/smallest-common-multiple-last-test-not-passing/281726

EDIT: here's one that doesn't pass master... https://www.freecodecamp.org/forum/t/smallest-common-multiple-code-breaking-down/299980

[RandellDawson]

@moT01 I just tested both posts listed on the topic you mention in your EDIT. The function returns 2018940 for the last test which is incorrect. It has nothing to do with triggering the infinite loop protection.

https://repl.it/@rmdawson71/incorrect-lcm

[ojeytonwilliams]

Edit: I've taken a closer look. Last December @ValeraS removed loop-protect from js challenges in https://github.com/freeCodeCamp/freeCodeCamp/pull/34524, so, as long as the code takes less than 5 seconds, it'll pass. If it does take too long, they get a message to that effect.

In other words, unless I'm missing something, the problem has been solved.

[moT01]

Here's what you get with an infinite loop on master...

And you do get the 5 seconds on each test before it times out. So yea, if everyone's fine with how it is - we can probably just leave this alone.

[RandellDawson]

I believe we are ok with closing this.