C2LC-224: Add tests for the AddNode aria-label generation

Description

We don’t currently have tests for the AddNode aria-label generation. Test cases to cover:

  1. Add at beginning of program
  2. Add at end of program
  3. Add between existing steps

Comments

  • Tony Atkins [RtF] commented 2020-09-22T08:24:52.335-0400

    @@Simon Bates, I’m looking through the code and trying to understand what you mean. I see the tests for the aria-label property in AddNode.test.js. It sounds like you want to add tests in ProgramBlockEditor.test.js to ensure that the aria labels are correct?

    https://github.com/codelearncreate/c2lc-coding-environment/blob/master/src/ProgramBlockEditor.js#L278

  • Tony Atkins [RtF] commented 2020-09-22T08:36:18.368-0400

    Looking at the coverage report, the function in question is exercised, but that doesn’t mean much unless we check the outcome.

  • Simon Bates commented 2020-09-22T08:41:01.635-0400

    Yes, that’s right. In AddNode.test.js we verify that the provided aria-label is rendered in the component. But we don’t have tests for the generation of the aria-label content, which is created in the function in ProgramBlockEditor.js that you link to.

    I think adding tests to ProgramBlockEditor.test.js would be the place to do it. We may want to refactor the aria-label generation out of ProgramBlockEditor, but I think leaving it where it is and testing it there is good for now. There will definitely be some refactoring of ProgramBlockEditor coming up, as that class is doing a little too much. But I’ve resisting making too many changes until we had a little more functionality in place and a better sense of the shape this part of the system should have.

  • Tony Atkins [RtF] commented 2020-09-22T08:43:24.416-0400

    I assume I’d add three tests similar to this one for scrolling when a step is added.

  • Simon Bates commented 2020-09-22T09:30:33.375-0400

    Yes. For each test situation:

    • Create an instance of ProgramBlockEditor with a suitable program
    • Find the AddNode(s) and verify that they have the expected aria-label

    Where we probably want the following test situations:

    • Empty program (expect one AddNode, which can append onto an empty program)
    • Program with one step (expect two AddNodes, one before and one after the step)
    • Program with two steps (expect three AddNodes, one before, one in between, and one after)

  • Tony Atkins [RtF] commented 2020-09-23T03:54:26.275-0400

    I think those tests have a fundamental flaw that I’ll need to fix while I’m working on this:

    https://github.com/codelearncreate/c2lc-coding-environment/blob/master/src/ProgramBlockEditor.test.js#L156

    Rather than actually returning the add button at a particular index, it’s hard-coded to zero. We mainly got away with it because we were adding a forward to a stack of forwards. I discovered it because I decided to add a left. It may take a little longer to unpack, I’ll share what I have if I need help fixing it.

  • Tony Atkins [RtF] commented 2020-09-23T04:15:46.931-0400

    OK, I suspected this was an issue before but thought maybe I was missing something. I just tried on someone’s instance to confirm. We can only add things at the beginning or in the middle if the toggle is clicked, and it’s off by default. For now I’ll make my tests click the toggle, but IMO it’s worth spending the small amount of time to make that configurable as part of the initial starting properties.

    This also explains why the old hard coding of the add button index to zero worked, as there was only ever one expanded add button.

  • Tony Atkins [RtF] commented 2020-09-23T05:47:35.952-0400

    https://github.com/codelearncreate/c2lc-coding-environment/pull/79

  • Tony Atkins [RtF] commented 2020-09-23T05:49:25.173-0400

    The pull is in, it adds aria label tests, and also tests for the expand toggle, and finally tests for adding nodes somewhere at the beginning and middle of a program.

  • Simon Bates commented 2020-09-23T08:48:48.109-0400

    I agree with regards to moving the AddNode expanded state up and providing it to the ProgramBlockEditor as a property. Could you write up a note on our Technical Debt doc? Thanks very much. In the future, we will want to persist user settings and I think the AddNode expanded state would be something that we would want to include.

    I initially created an object within the AppState called ‘settings’ of type AppSettings with the intention of using that as the place to locate settings that we want to persist, but I didn’t go very far with it yet. But I think that would be the place to put it.

    And thanks very much for finding and fixing the issue with the ignored index in getAddNodeButtonAtPosition().