Code review comments

I enjoy code reviews, especially if it's with colleagues I've been a mentor to. This is a list of linkable comments for past, present or feature mentees.

Note - Our team works asynchronously in very different time zones, so the comments are more detailed. In distributed teams, I've found that targeted over-sharing can be productive.

Namespacing

Scenario

There is a TypeScript string enum in the codebase that houses various attribute selectors used for Cypress tests:

export enum DataCySelectors {
  modal_title = 'modal-title'
}

and the author creates a new property, submit_button_modal.

Comment

Change submit_button_modal to modal_button_submit. Some reasons why we have this convention (largest context to smallest, left to right) are:

• We read left to right, so it's faster at a glance to see what context the namespaced item is associated with (e.g. there can be many submit buttons but there's only one main modal component)
• We visualize the DOM in a top-down hierarchy, so it's faster to match the namespaced item with our mental model
• Allows us to group the namespaced items together in an alphabetical list (e.g. all the modal- items are together [which is useful], as opposed to all the submit buttons being together [which is less useful])
• VSCode's intellisense will be much more useful since we're more likely to type DataCySelectors.mo looking for modal-related selectors and the modal-related selectors will actually show up in the context menu

DRY integration tests

Scenario

The author creates a generically named utility function that is called in more than one test in an effort to make tests more DRY.

Comment

Consider how to improve the ergonomics of these test utility functions. For example, the name of this function is selectProduction but looking at the function declaration, it can/should only be used in the dashboard. A couple options for how to do this:

• Namespace functions
• Group export functions as object properties, e.g:

export const CyDashboardUtil = {
  selectProduction,
}

and use it l like:

import { CyDashboardUtil } from '../utils';

it('should do something', () => {
  CyDashboardUtil.selectProduction();
  ...
  // The rest of your test
});

For clarity, I think a healthy convention for us is to think of custom Cypress commands as fragments of functionality that can be used in all tests, whereas these utils are fragments of functionality that can be used in a subsection of tests.

Before creating either a custom command or a util, it's a good idea to carefully consider the tradeoffs (which I think you've done a good job of in these later commits). For example, if (and when) this test breaks:

• Is the context switch the engineer will have to make to go look at the command/util function declaration more or less costly than not creating the command/util and repeating yourself in the test body?
• Is there a chance that the engineer will not be able to tell what line the test actually failed because the failure occurred in the command/util?
• Is the fragment of functionality actually specific to that particular test (so it shouldn't be a command/util)?
• Is there any way I can alter my assertions to make them more concise and avoid extracting out a new command/util?

If after all these considerations you think it's still worth it, then go ahead. Keep in mind though that the best tests are easy to read with no prior context and quick to fix, so extracting stuff out can easily throw a wrench in those goals.

Rebasing

Scenario

After a series of reviews that require changes, the author has added multiple commits and merged another branch into his.

Comment

Can you rebase your branch on canary please, it's hard to see which changes are yours.

You may know how to do this already, but just in case here are the steps:

1. git checkout master, then git pull to get the latest master changes
2. git checkout [your-branch-name], then git rebase -i master and follow the prompts
3. When done, git branch to make sure you're still on your branch, then git push -f to overwrite your old remote branch history with your newly squashed branch history

When rebasing you should:

• Squash all commits that are not substantial (e.g. test(fix): pull and merge) or commits that should be a single commit
• Edit commit messages (e.g. test(fix): implement requested changes should instead describe what those changes are)
• Resolve conflicts if conflicts occur (the trickiest part, can do it in VSCode)

If you screw up in the middle of it (we all do this), you can close the terminal, open a new one and git rebase --abort to undo everything. If you're especially worried you can always make a local duplicate of your branch before rebasing as backup (your history will still be on your remote branch until you force push but sometimes its helpful to have insurance).

You can also do this whole process with whatever git client you use but I like the built in interactive cli rebase.

Destructuring variables

Scenario

A variable is declared:

const comment = ['Some comment text'];

and later referenced with comment[0].

Comment

Consider how we can make these local variables more convenient to use. If there will be only one value you can initialize the variable with a string value, but if you expect there to be more than one value you can communicate more clearly by using specifically named variables (instead of having to reference it like comment[0]). Some options:

• Use an object, like:

const comment = {
  someUsefulKeyName1: 'First comment',
  someUsefulKeyName2: 'Second comment',
};

and reference like comment.someUsefulKeyName1, or the fancy destructured way:

const { someUsefulKeyName1, someUsefulKeyName2 } = {
  someUsefulKeyName1: 'First comment',
  someUsefulKeyName2: 'Second comment',
};

and use the properties directly (someUsefulKeyName1).

• If you want to keep the array, you can also destructure the array (which assigns named local variables in the process):

const [someUsefulKeyName1, someUsefulKeyName2] = ['First comment', 'Second comment'];

Keep in mind though that unlike object destructuring, array destructuring depends on the order of the array items. This makes it generally less used.

Thanks for reading! Go home for more notes.