fix(test): Throw if elements are not found #1344

lasjorg · 2021-08-03T10:35:21Z

Pre-Submission Checklist

Your pull request targets the master branch of freeCodeCamp/testable-projects-fcc
Branch starts with either fix/, feature/, or translate/ (e.g. fix/signin-issue)
You have only one commit (if not, squash them into one commit).
Your changes have been tested either locally or using a newly created CDN based on your fork's testable-projects-fcc/build/bundle.js file

Type of Change

Small bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds new functionality)
Breaking change (fix or feature that would change existing functionality)
Add new translation (feature adding new translations)

Checklist:

Tested changes locally.

Closes currently open issue: Closes #XXX

Description

Right now tests 6 and 7 will pass with no code for the "Build a Random Quote Machine" project. Not sure what the best way of handling this is but I think just throwing in an else block if the elements are not found should work just fine.

Forum post: https://forum.freecodecamp.org/t/build-a-random-quote-machine-bug-in-test/472095

src/project-tests/quote-machine-tests.js

ShaunSHamilton

This is more what I mean. I am just a fan of less code 🤷‍♂️

ShaunSHamilton · 2021-08-03T20:18:40Z

src/project-tests/quote-machine-tests.js

+        } else {
+          assert.exists(text, 'element with id="text" not found');
        }


Suggested change

} else {

assert.exists(text, 'element with id="text" not found');

}

}

assert.exists(text, 'element with id="text" not found');

src/project-tests/quote-machine-tests.js

lasjorg · 2021-08-07T10:18:54Z

@ShaunSHamilton Sorry for the lack of update on this.

I had it working to where the example project and the empty project passed and failed respectively as needed. But then I tested with a React version of the project I had laying around and it fails. I rolled back to the previous code and it still fails some of the tests. It seems this PR broke it because the project passes with the old test code. I get the feeling the React conditional rendering I have in the project doesn't work with the new code.

I might play around with it some more at some point, but right now I don't really have anything.

ShaunSHamilton · 2021-08-07T13:59:42Z

@lasjorg The only thing that comes to my mind that could be an issue with the linked PR's changes is if the script is added before the body of the HTML.

In your old React form of the project, how are you adding the test suite?

lasjorg · 2021-08-08T14:01:13Z

@ShaunSHamilton No that's not it. As I said, it still passes with the old test code from before that PR.

Here is a replit, you can switch the test scripts in the HTML file (current and old).
https://replit.com/@lasjorg/random-quote-tests

ShaunSHamilton · 2021-08-08T14:21:18Z

Just to mention it, if you were unaware: Removing the hasLoaded in your app allows it to pass the tests:

return (
    <>
-       {quotes && hasLoaded && (
+      {quotes && (
        <div id="quote-box" className={`App ${hasLoaded && 'visible'}`}>
          <Quote title={quotes.author} content={quotes.content} />
          <Buttons
            handleClick={handleClick}
            disabled={!hasLoaded}
            href={getTweetLink(quotes.author, quotes.content)}
          />
        </div>
      )}
    </>
  );

I also renamed onClick as a prop, because it was confusing me 🤫

lasjorg · 2021-08-08T16:08:52Z

Right but as I said it's just some code that fails with the current test that didn't with the old. It wasn't about changing the code to make it pass it was just an example of code that used to pass that now fails.

I will just push what I have now because I started to mess around with it and I'm losing track of what I have tested. I added a test for the width of the container to not allow for full-width containers counting as centered. The current code should pass the example project, fail the empty project, and pass the React project with the change to the conditional code as you suggested.

Having all that code before the promises just feels off to me, anyway it's what I have got so far so I might as well just push it.

ShaunSHamilton

This still causes your example app to fail, unless the hasLoaded logic is removed from the render in App.js.

So, I am fine with just going ahead with this for better assertion messages. Is that what we want?

lasjorg · 2021-10-18T22:51:33Z

This still causes your example app to fail, unless the hasLoaded logic is removed from the render in App.js.

As I said.

The current code should pass the example project, fail the empty project, and pass the React project with the change to the conditional code as you suggested.

I'm still not sure what change was made in the other PR I linked to that is causing the old code to fail. But I'm not sure if it matters?

So, I am fine with just going ahead with this for better assertion messages. Is that what we want?

Well, the main point of this PR was to fix empty projects passing tests 6 and 7, and as later pointed out by the user in the forum, also fix the layout test for centering. If the assert messages are better as well that's just a bonus.

It still feels a little awkward having all this synchronous code before the promise code, but I'm not sure what else to do.

ShaunSHamilton

Thank you, for baring with me, @lasjorg .

I am happy with what this accomplishes.

Something for the next reviewer: Is there any reason the 25px leeway is too little for the testHorizontallyCentered function?

moT01

I was struggling with testing this - finally figured it out. I made two suggestions.

src/project-tests/quote-machine-tests.js

lasjorg · 2021-10-26T17:26:07Z

Sorry for the delay on this.

@moT01 You are right, my bad. I think when testing this I had misunderstood what the fix from the other PR was doing. For some reason, I was under the impression that it was checking for a new quote and not just re-clicking the button if the quote was the same. So I guess when I saw the tests passing I didn't think much of it.

I had to rebase with main to get this to run again, so rebase > force push

lasjorg · 2022-01-30T22:24:47Z

@moT01 I forgot all about this PR. Are there any other issues that need to be addressed here?

A forum post that made me remember this PR. This project can be passed with an almost empty project as it is now.

https://forum.freecodecamp.org/t/im-getting-12-12-tests-passed-on-random-quote-machine-but-ive-barely-written-7-lines-of-code/494900

moT01

LGTM @lasjorg 🎉 Sorry for the delay here - Thanks for the work on this and all your contributions 🎉

lasjorg requested a review from a team as a code owner August 3, 2021 10:35

ShaunSHamilton reviewed Aug 3, 2021

View reviewed changes

src/project-tests/quote-machine-tests.js Outdated Show resolved Hide resolved

ShaunSHamilton suggested changes Aug 3, 2021

View reviewed changes

naomi-lgbt requested a review from ShaunSHamilton October 18, 2021 18:22

ShaunSHamilton reviewed Oct 18, 2021

View reviewed changes

ShaunSHamilton approved these changes Oct 19, 2021

View reviewed changes

moT01 reviewed Oct 20, 2021

View reviewed changes

src/project-tests/quote-machine-tests.js Outdated Show resolved Hide resolved

src/project-tests/quote-machine-tests.js Outdated Show resolved Hide resolved

lasjorg added 4 commits October 26, 2021 19:07

fix(test): Throw if elements are not found

c61a56c

fix(test): update assert method

4dc11a8

add: update tests

f4e219e

fix(test): move button click

cf66407

lasjorg force-pushed the fix/throw-failure-if-no-elements-are-found branch from 3d84193 to cf66407 Compare October 26, 2021 17:21

moT01 self-assigned this Dec 15, 2021

ShaunSHamilton mentioned this pull request Jan 31, 2022

Random Quote Machine template passing 12/12 test with barely any code freeCodeCamp/freeCodeCamp#44972

Closed

moT01 approved these changes Mar 11, 2022

View reviewed changes

moT01 merged commit 15d9a9f into freeCodeCamp:main Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(test): Throw if elements are not found #1344

fix(test): Throw if elements are not found #1344

lasjorg commented Aug 3, 2021

ShaunSHamilton left a comment

ShaunSHamilton Aug 3, 2021

lasjorg commented Aug 7, 2021

ShaunSHamilton commented Aug 7, 2021

lasjorg commented Aug 8, 2021

ShaunSHamilton commented Aug 8, 2021

lasjorg commented Aug 8, 2021

ShaunSHamilton left a comment •

edited

lasjorg commented Oct 18, 2021

ShaunSHamilton left a comment

moT01 left a comment •

edited

lasjorg commented Oct 26, 2021

lasjorg commented Jan 30, 2022

moT01 left a comment

fix(test): Throw if elements are not found #1344

fix(test): Throw if elements are not found #1344

Conversation

lasjorg commented Aug 3, 2021

Pre-Submission Checklist

Type of Change

Checklist:

Description

ShaunSHamilton left a comment

Choose a reason for hiding this comment

ShaunSHamilton Aug 3, 2021

Choose a reason for hiding this comment

lasjorg commented Aug 7, 2021

ShaunSHamilton commented Aug 7, 2021

lasjorg commented Aug 8, 2021

ShaunSHamilton commented Aug 8, 2021

lasjorg commented Aug 8, 2021

ShaunSHamilton left a comment • edited

Choose a reason for hiding this comment

lasjorg commented Oct 18, 2021

ShaunSHamilton left a comment

Choose a reason for hiding this comment

moT01 left a comment • edited

Choose a reason for hiding this comment

lasjorg commented Oct 26, 2021

lasjorg commented Jan 30, 2022

moT01 left a comment

Choose a reason for hiding this comment

ShaunSHamilton left a comment •

edited

moT01 left a comment •

edited