JS Calculator Test Issue on React

JavaScript
1

Greetings,

I’ve coded the JS Calculator challenge in React. Manually it passes all the included tests however it only passes 8 out of 16 of the tests automatically.

I’m thinking that the issue might stem from the fact that the useState hook is inherently asynchronous while the tests expect synchronous behavior. However, useState is such a basic part of modern React - I’d be very surprised if that’s indeed the issue. I tried updating the HTML of the element with the id “clear” to “0” to test out if this is indeed the issue, however I failed the test again.

I’d appreciate any help with this issue. Here is the link to my codepen page:

Thanks in advance!

Your browser information:

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:92.0) Gecko/20100101 Firefox/92.0

Challenge: Build a JavaScript Calculator

Link to the challenge:

2

True, but so is setState. And a lot of people have done this project with hooks.

3

This is the first test you are failing:

At any time, pressing the clear button clears the input and output values, and returns the calculator to its initialized state; 0 should be shown in the element with the id of “display”

Element with with id=“display” should show 0 : expected ‘5’ to equal ‘0’
AssertionError: Element with with id=“display” should show 0 : expected ‘5’ to equal ‘0’

For reference, if I go to the the repo for the tests, this is the test that is failing:

      it(`At any time, pressing the clear button clears the input
      and output values, and returns the calculator to its initialized state; 0
      should be shown in the element with the id of "display"`, function () {
        clickButtonsById([_5, _x, _1, _plus, _5, _plus, _9, _2, _eq, _AC]);
        assert.strictEqual(
          getInputValue(document.getElementById('display')),
          '0',
          'Element with with id="display" should show 0 '
        );
      });

So, it is looking for the string '0'.

This is the the relevant code in your pen:

  function Result({ content }) {
    return (
      <div id="display" className="display">
        <p>{content}</p>
      </div>
      )
  }

First of all, I’m not sure if it is OK for the value to be in the child of the element with the id “display”. But even if that is not the issue, when I log out content:

  function Result({ content }) {
    console.log('content', content)
    return (
      <div id="display" className="display">
        <p>{content}</p>
      </div>
      )
  }

I see that it is an array. Why is it an array?

See if you can work through the test report and figure out what is going wrong. These are important skills for developers.

1 Like
4

Thank you for checking my code, Kevin!

For the tests and styling, having the id “display” in the child is fine. That also worked in the previous challenges. Actually I recently learned that components do not play well with id and class names as props, especially with the map method.

Arrays seemed logical to me, especially for the formula approach. And the project didn’t require the data type for the states to be non-array types.

For sake of testing I also changed the state to a string and rewrote the clear function. Didn’t pass the test.

I think what’s really going on with the tests is that they are not waiting for React’s state management. React waits until the next re-render to update the state for efficiency. However from what I see the test suite simply is not suited for React’s state update approach.

There are a few people on this forum who employed the synchronous counterpart of useEffect, namely useLayoutEffect. But I think this shouldn’t be a requirement, especially when the use of modern React is encouraged in this challenge.

5

Again, you’re focussed on the hooks. Hooks are not the problem. Asynchronous is not the problem. useState is no more async than setState and this project has been done many times with each.

6

I’m trying to figure out the source of the discrepancy between the tests and my code. The async approach of React states is the first and honestly only thing I can think of as the source of the issue, based on my debugging. If there is any concrete reason in my code that anyone can point out I’d appreciate it greatly.

My implementation is functional and manually passes almost all of the tests. I am in line with the steps of the user story. I am simply almost clueless where the real issue is.

7

I think you have some issues. When I put a log statement in your App, before the return:

  // ...
  console.log('input', input, typeof input)
  
  return (
    <div className="container">
    // ...

When I run the test suite, I get the console output:

input 0 string
input [‘5’] object
input 0 string
input [‘1’] object
input 0 string
input [‘3’] object
input 0 string
input [‘0’] object
input 0 string
input [‘5’] object
input 0 string
input [‘1’] object
input 0 string
input [‘5’] object
input 0 string
input [‘5’] object
input 0 string
input [‘2’] object
input 0 string

So, something odd is going on here. I would want to figure that out.

8

Thanks for pointing that issue out, Kevin. I’ll look into this and give an update once I have something.

1 Like
9

Time for an update! :slight_smile:

The problem was not that I used an array to maintain state, nor was it asynchronicity of React’s state management (thanks again for pointing that out, Kevin). It had something to do with timing, though: I had created the Display and Cell components with the main Calculator component. This messed up the tests because with each re-render these sub-components were created from scratch. Although this did not create any functional issues, it made my code prone to bugs and interfered with the tests.

Instead, I moved the component Display and Cell component declarations outside the Calculator component, and voila, the app passed 15 of the tests. Lastly, I implemented a solution for the 13th test, and now I have passed all the tests.

Thanks again, Kevin!

10

I’m sorry, but this was definitely a problem. There’s nothing wrong with having an array in state, but the fact that you were alternatively sending in an arrays and strings is very, very, very concerning. Clearly your output was expecting a string (you don’t render arrays of strings to the screen). This kind of confusion about what a variable is can lead to very serious bugs. If that code came across my desk for review, I would reject it automatically, once I saw that.

One of the complaints people have about JS is a lack of type safety. That is why things like Flow and TypeScript were invented. Without those, you have to be very sure to maintain some type discipline and also make sure that you completely understand what your variables are. It is not good practice to have a variable that spontaneously changes types like that, from array to string.

What you have now:

function Result({ content }) {
  return (
    <div id="display" className="display">
      <p>{ content }</p>
    </div>
  )
}

At least now it is consistent in that it getting an array. But I also don’t like the idea of sending an array of strings to the DOM like that. I don’t know if that is defined behavior for React and JS. I would want to do that “properly” by joining it into a string myself.

Also, the use of a p here seems odd to me. Semantically, that is for a paragraph - that is not what this is. It is also a block-level element and that is not really what this is. I would have expected either a span or just use the surrounding div (though I probably would have made that a span).

I don’t know, it just seems odd to me. Writing code isn’t just about getting it to work. It’s also about writing clean, safe, maintainable, readable code. I know, I probably wasn’t thinking too much about that when I was where you are. But it will become important later.

11

What I meant by the issue was the discrepancy between functionality and the inner working of the app. The reason this happened was that I created the sub-components inside the main component. Now I know that this leads to many problems and is an anti-pattern.

I was not randomly setting up the state and changing types during the flow of information within the app. The time when you checked my code I was probably experimenting with the code, hence the type confusion. The way I designed the code was not to use strings during the composition of the formula. Of course I appreciate your warnings about JS’s implicit type conversion.

Joining the array before rendering absolutely makes sense. I’ve just implemented that.

As for the use of the “p” element, I am not convinced that this constitutes an issue. I don’t see a problem with the Display component utilizing a block-level element. Actually I don’t see why inline-level elements would even make sense here. The result line will not share its block space with any other element.

Apparently, just using the surrounding div for text should be the last resort, according to W3 (HTML Standard).

Lastly, my goal is never just to make things work and ignore the inner-workings of programming paradigms. My background is in a field that spaghetti code has been the norm, and I have always strongly disliked the “just-make-it-work-and-forget-about-the-reasons-why-things-might-fail” approach. Anyway, I’m glad that I’ve learned a lot from this challenge and I’m hoping to grow.

Also, here is the code from the testing suite in case anyone from the future visits here :slight_smile:

12

OK, then, best of luck on your coding journey.

1 Like
13

Are you going to fail an accessibility audit for it? Probably not. Is it semantically the best choice? Probably not. The <p> element is generally meant to markup a paragraph of text. Granted, paragraph can be quite a few different things but in this case I don’t think the numerical output of a calculator would be one of those.

There is nothing wrong with just having the <div> be the display element. There is an <output> element, which seems like it might be the best fit, but it does have some accessibility issues that you need to be aware of. Some screen readers will automatically treat it as a live region, which means that any text changes in the element will be automatically read out loud. I think this might be OK if you used it only to display the final result of a calculation. But you are using the same element to both mirror the buttons pressed and to display the result, so you definitely don’t want to use <output> in this scenario.

Yes, the note on the standard you linked to does say to use a div as a last resort, but then it continues “for when no other element is suitable.” HTML is great but there is definitely not an element for everything and in this case I don’t think <p> fits here, and <output> has its problems, so <div> is probably the best choice in this specific scenario. I think if you did a quick audit of other web-based calculators out there you would find very few that use a <p> for this.

Update: By the way, speaking of accessibility, I think you would definitely want a screen reader to announce the results of a calculation automatically using a live region so the user doesn’t have to manually find it. So perhaps using <output> might be the best solution but you would need to make the display that mirrors the buttons being pressed a separate element for reasons mentioned above.

2 Likes
14

Yeah, I just don’t like a div because to me that represents a section, a division between sections. It usually adds a line break afterwards because of this. It is a block-level element.

If I look up div, I get:

The <div> tag defines a division or a section in an HTML document.
The <div> tag is used as a container for HTML elements - which is then styled with CSS or manipulated with JavaScript.

And for span is an inline element, I get:

The <span> tag is an inline container used to mark up a part of a text, or a part of a document.

Between those two, I think the latter is a better fit for what is happening on this page.

But you point out output:

The <output> tag is used to represent the result of a calculation (like one performed by a script).

I agree that that might be a semantically better fit.

I’m not saying I think it’s terrible to use a div there, but for me it just seems a little out of place. But I’m sure it gets used a lot for things like this, and I’ve probably done it too. But the p, that definitely seemed out of place for me.

1 Like
15

Thank you so much for the detailed explanation of the accessibility and semantic issues, bbsmooth. Now I see how the p tag could be quite problematic.

16

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.