Add tests for RB.UploadAttachmentView

Review Request #10941 — Created March 7, 2020 and updated

hxqlin
Review Board
release-3.0.x
dd4c535...
reviewboard

This patch adds unit tests for RB.UploadAttachmentView. The recent refactoring
of the view led to some regressions that were not caught until the changes were
already landed. With unit tests, we can help reduce the likelihood of this
happening in the future.

New tests:
- Cancel button has the correct state when the dialog is initialized
- Cancel button closes the dialog when clicked
- Upload button has the correct state when the dialog is initialized
- Upload button has the correct state when a file is uploaded and when it is
removed
- Upload button sends request to create a file attachment when clicked
- the dialog is populated with the right information when updating a file
attachment

Description From Last Updated

If this function is empty, could we remove it?

bui1bui1

Change this to UploadAttachmentView instances

bui1bui1

Remove the blank line

bui1bui1

Turn to Cancel Button instead. Both David and Christian stress that test descriptions should be as clear as possible so ...

bui1bui1

Turn this to Upload Button

bui1bui1

Minor code styling issue to fix, just need to line up the line like this below spyOn(dialog, 'updateUploadButtonEnabledState') .and.callThrough();

bui1bui1

Maybe change this to Disabled by default until a file is uploaded to make it clear it is disabled by ...

bui1bui1

Since expect(dialog.updateUploadButtonEnabledState).toHaveBeenCalled(); is being called more than once in this test function, maybe be more specific with how many times ...

bui1bui1

This should be rb/views/UploadAttachmentView (class capitalization).

chipx86chipx86

For ES6 JavaScript, the last item in a list or object should have a trailing comma.

chipx86chipx86

When using let or const, we should list one variable per statement. So: let dialog; let $caption; let $attachmentHistory; Alphabetical ...

chipx86chipx86

Blank line between these. Though, it'd probably make more sense to move dialog = null into the conditional.

chipx86chipx86

Only one test needs this, so it should move into that test.

chipx86chipx86

Since this is a precondition sanity-check for test setup, it should move into beforeEach()

chipx86chipx86

One per statement.

chipx86chipx86

Multi-line comments should be in the form of: /* * ... * ... */ Each comment should also have proper ...

chipx86chipx86

Single quotes are preferable so long as content doesn't need to use a single quote.

chipx86chipx86

Similarly, this should move into beforeEach().

chipx86chipx86

This is going to result in something like Blah blah blah blah This really sounds more like two tests. I'd ...

chipx86chipx86

This should use single quotes.

chipx86chipx86

I'd suggest chaining the statements: $path .val('') .trigger('change'); Same with similar code paths below.

chipx86chipx86

This statement exceeds the max line length limit. You should wrap at .toHave.....

chipx86chipx86

This should use single quotes.

chipx86chipx86

This statement exceeds the max line length limit.

chipx86chipx86

This statement exceeds the max line length limit. Also, the context for this test is "Buttons -> Upload", so we ...

chipx86chipx86

Can you inverse the types of quotes: '[name="caption"] is more consistent with our standards.

chipx86chipx86
bui1
  1. Out of curiosity, is there a reason why dlg.show() is used instead of dlg.render() and then dlg.show()?

    Some things you can try,
    - playing around between spyOn(view, '_someFunc').and.callThrough(); and spyOn(RB.SomeView.prototype, '_someFunc').and.callThrough();. I found that the former is good for testing elements that are child elements of the parent view that is rendered. The latter is good for testing a function that is within the same parent view you're testing.
    - using view.delegateEvents(); if there's child element functions to test. It looks like you're testing with an input (child) within the parent(uploadAttachmentView), this looks like a good case to use it. You would call delegateEvents after initializing the spy.

    Example,

         // render the parent view
    
         // _onSelectDiffFileKeyDown is triggered by a specific button(child). $path would be the child in this scenario
         spyOn(parentview, '_onSelectDiffFileKeyDown').and.callThrough();
    
         // make sure child functions will be called
         parentview.delegateEvents();
    
         const selectDiffFileButton = parentview.$el.find('#select-diff-file');
         keyEvent.which = $.ui.keyCode.ENTER;
         selectDiffFileButton.trigger(keyEvent);
         expect(parentview._onSelectDiffFileKeyDown).toHaveBeenCalled();
    

    • I know Christian said order doesn't matter when setting up the spies (but it did for me :P ). If you're using spyOn(view, '_someFunc').and.callThrough();, a view should be rendered afterwards. For spyOn(RB.SomeView.prototype, '_someFunc').and.callThrough(); the view should be rendered before the spy is created.
    • Using $testsScratch to make sure the dialog is actually being attached to the DOM.
    • Maybe instead of directly using $path.trigger('change'), declare a event = $.Event('change'); and pass that in to trigger or triggerHandler instead? Not sure if that'll work

    I know its not a finished review request yet but also changing dlg to dialog will help out too as I didn't get that abbreviation right away.

    1. Oops for this point I know Christian said order doesn't matter when setting up the spies (but it did for me :P ). If you're using spyOn(view, '_someFunc').and.callThrough();, a view should be rendered afterwards. For spyOn(RB.SomeView.prototype, '_someFunc').and.callThrough(); the view should be rendered before the spy is created.

      it should be

      I know Christian said order doesn't matter when setting up the spies (but it did for me :P ). If you're using spyOn(view, '_someFunc').and.callThrough();, a view should be rendered **before** then create the spy. For spyOn(RB.SomeView.prototype, '_someFunc').and.callThrough(); the spy should be created then render the view

    2. Thanks again for the tips! To answer your question about show - show() calls render() and does some other setup that I think isn't supposed to be included in render() since render's default behavior is to do nothing and return the object for chaining.

  2. 
      
hxqlin
hxqlin
bui1
  1. Just some minor things to address but otherwise the test contents itself are solid :)

    1. Thanks! I definitely get your concerns about naming tests more clearly but I'm not sure some of your suggestions would apply here - naming specs the way I've done here allows us to take advantage of Jasmine's concatenation of spec names to fully describe each test. In my case, the test names looks like this:

      rb
       views
         uploadAttachmentView
           Instances
             Buttons
               Cancel
                Enabled by default
                Closes dialog when clicked
               Upload
                Disabled until a file is uploaded
                Becomes enabled when a file is uploaded and disabled when the uploaded file is removed
                Clicking upload sends request to create a file attachment
           Updating file attachment
      

      At each test, we can see what this is testing pretty clearly by going up the tree. Changing Cancel to Cancel button, for example, would introduce unnecessary redundancy.

  2. If this function is empty, could we remove it?

  3. Change this to UploadAttachmentView instances

  4. Turn to Cancel Button instead.

    Both David and Christian stress that test descriptions should be as clear as possible so it's easier to traverse through errors when the entire suite runs.

  5. Minor code styling issue to fix, just need to line up the line like this below

    spyOn(dialog, 'updateUploadButtonEnabledState')
         .and.callThrough();
    
    1. I think that's actually indented a little too far - see other examples like in postCommitViewTests.js:

      spyOn(RB.PostCommitView.prototype, '_showLoadError')
          .and.callThrough();
      
  6. Maybe change this to Disabled by default until a file is uploaded to make it clear it is disabled by default

  7. Since expect(dialog.updateUploadButtonEnabledState).toHaveBeenCalled(); is being called more than once in this test function, maybe be more specific with how many times this is being called.

    expect(dialog.updateUploadButtonEnabledState).toHaveBeenCalledTimes(1);

    or instead at the very end of the function,

    expect(dialog.updateUploadButtonEnabledState).toHaveBeenCalledTimes(2);

  8. I'm kind of unclear on what this test is supposed to do. Correct me with I'm wrong.

    • Show a dialog with preloaded information
    • Check if caption and history has been updated based on this preloaded information?

    Initially at first I thought this test would check if the caption got updated to a new value different from the preloaded one or the content of the dialog changes and we check if the history got incremented by one.

    1. This test is to check that if a dialog is triggered with prepopulated information, it actually displays that information. This was one of the regressions caused by the refactoring. Although the other possible test cases you bring up are definitely valid, the focus of this test is just to capture whether the dialog is rendered correctly if attachmentHistoryID and presetCaption are prepopulated. What do you think would be a clearly way of describing that?

  9. 
      
hxqlin
chipx86
  1. 
      
  2. This should be rb/views/UploadAttachmentView (class capitalization).

  3. For ES6 JavaScript, the last item in a list or object should have a trailing comma.

  4. When using let or const, we should list one variable per statement. So:

    let dialog;
    let $caption;
    let $attachmentHistory;
    

    Alphabetical order is always appreciated.

  5. Blank line between these.

    Though, it'd probably make more sense to move dialog = null into the conditional.

  6. Only one test needs this, so it should move into that test.

  7. Since this is a precondition sanity-check for test setup, it should move into beforeEach()

  8. Multi-line comments should be in the form of:

    /*
     * ...
     * ...
     */
    

    Each comment should also have proper sentence capitalization and punctuation.

  9. Single quotes are preferable so long as content doesn't need to use a single quote.

  10. Similarly, this should move into beforeEach().

  11. This is going to result in something like Blah blah blah blah

    This really sounds more like two tests. I'd have one that tests just setting the file and asserts around that, and a second that sets things up by setting the file and then tests the removal case.

  12. This should use single quotes.

  13. I'd suggest chaining the statements:

    $path
        .val('')
        .trigger('change');
    

    Same with similar code paths below.

  14. This statement exceeds the max line length limit. You should wrap at .toHave.....

  15. This should use single quotes.

  16. This statement exceeds the max line length limit.

  17. This statement exceeds the max line length limit.

    Also, the context for this test is "Buttons -> Upload", so we can get rid of the "Clicking upload ...". Maybe just make this "Uploads the file attachment".

  18. Can you inverse the types of quotes: '[name="caption"] is more consistent with our standards.

  19. 
      
hxqlin
Review request changed

Change Summary:

Fix issues from review

Commit:

-6a631b1b126147584a38139b2f5e5798747d59c5
+dd4c53527a8a58d7302a39bc0691386f1afb4831

Diff:

Revision 5 (+140)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...