New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strict Null Checking Part II: The Testing! #65233

Closed
mjbvz opened this Issue Dec 17, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@mjbvz
Copy link
Contributor

mjbvz commented Dec 17, 2018

A sequel to October's hit: #60565, Strict Null Checking VS Code

Backstory

We are incrementally working to enable TypeScript's strict null checks for the core of VS Code: #60565. Strict null checks help to catch silly programming mistakes and generally enforce a more explicit and safer coding style. Enabling strict null checks should reduce the number of undefined access exceptions VS Code users hit and help VS Code contributors move more quickly

This time

With #60565, we have already migrated over 800 files to be strict null checked but have not yet enabled strict null for many of our unit tests. That's where you can help out.

This item tracks enabling strict null checks for the *.test.ts files in the core VS Code codebase (under ./src). This work can be done on a file-by-file basis and should be fairly straightforward. If you are interested in contributing to VS Code, enabling strict null checking for a few test files is a great way to help out and a great way to learn more about the codebase.

How to contribute

  1. Setup a VS Code development environment if you have not already done so: https://github.com/Microsoft/vscode/wiki/How-to-Contribute

  2. Make sure you are on the latest commit of VS Code from master.

  3. Pick a strict null check eligible test file from the list posted below.

  4. Add the file in the files section of the src/tsconfig.strictNullChecks.json file in the VS Code codebase.

    "files": [
        ...,
    
        // To avoid merge conflicts, try to add your file in roughly sorted order
        "./vs/base/test/common/keyCodes.test.ts",
    
        ...
    ]
  5. Run yarn strict-null-check, or yarn strict-null-check -- --watch to run the null checks in watch mode

  6. Fix null check related errors. See the guidelines below for more details. Some files may only have a single error while others may have hundreds. We welcome all incremental fixes

  7. Run the tests to make sure they still pass

  8. Submit a PR with your change. Try to keep each PR to converting a single file

General guidelines for fixing strict null errors in tests

  • Annotate nullable types and fix simple type errors

  • Use the ! not null assertion to skip null checking when testing APIs that may return undefined. For example, if we were testing the divide function:

    function divide(a: number, b: number): { result: number } | undefined {
        return b === 0 ? undefined : { result: a / b };
    }
    
    test('div tests', () => {
        // Normally strict null checks would complain about accessing `.result` since
        // `divide` may return undefined. Use the ! not null assertion to suppress this
        // since the test will fail anyways if we try accessing `undefined.result`. 
        assert.strictEquals(divide(1, 2)!.result, 0.5);
    });

❗️ If you aren't sure about how to fix a given error, just ask or leave it unfixed. We can only merge PRs that have a passing build so if you do skip any errors, make sure your PR does not actually add the file to src/tsconfig.strictNullChecks.json.

(Also please do not close this issue in your PR. This issue will be left open until we have enable strict null checking for all tests)

@mjbvz

This comment has been minimized.

Copy link
Contributor Author

mjbvz commented Dec 17, 2018

List of eligible test files

Updated: December 24, 2018

  • "./vs/editor/contrib/find/test/findModel.test.ts"
  • "./vs/editor/contrib/suggest/test/suggestMemory.test.ts"
  • "./vs/platform/commands/test/commands.test.ts"
  • "./vs/platform/keybinding/test/common/abstractKeybindingService.test.ts"
  • "./vs/workbench/parts/search/test/common/searchModel.test.ts"
  • "./vs/workbench/parts/search/test/common/searchResult.test.ts"
  • "./vs/workbench/parts/tasks/test/electron-browser/configuration.test.ts"
  • "./vs/workbench/services/files/node/watcher/unix/test/chockidarWatcherService.test.ts"
  • "./vs/workbench/services/keybinding/test/macLinuxKeyboardMapper.test.ts"
  • "./vs/workbench/services/search/test/node/search.test.ts"

@mjbvz mjbvz pinned this issue Dec 17, 2018

@mjbvz mjbvz added this to the On Deck milestone Dec 17, 2018

@joaomoreno joaomoreno unpinned this issue Dec 18, 2018

@punteek

This comment has been minimized.

Copy link
Contributor

punteek commented Dec 21, 2018

Hi, I'm fairly new to open source contribution, so I though I'd start here by tackling "./vs/base/test/node/flow.test.ts". I noticed that the first strict null checking error is due to a callback parameter being null while it should be of type Error according to the function definition. How would I go about solving this issue? It doesn't seem like this is something I can fix by doing something like null!. Thanks for the help!

@jkmdev

This comment has been minimized.

Copy link
Contributor

jkmdev commented Dec 21, 2018

@punteek tbh I just figured it out, I'm about to make a PR for it now.

@piraces

This comment has been minimized.

Copy link

piraces commented Jan 18, 2019

Hello.
This is my first time contributing to vscode. I have searched for "good first issue" issues, and I came here. I'm going to start here by contributing on "./vs/editor/contrib/suggest/test/suggestMemory.test.ts" (if it's welcomed) 😄

Edit: Seems that @isidorn has fixed the only error it has in 442cf88. I was working on it but nevertheless I'm going to submit my PR to add the file in the "tsconfig.strictNullChecks.json".

@Cavonte

This comment has been minimized.

Copy link

Cavonte commented Jan 19, 2019

Hello,
I was hoping to contribute but as far as I can tell, the remaining test files
have been added to the tsconfig.strictNullCheck.json file on master.
The strict null check found 0 errors.
i.e.
[x] "./vs/editor/contrib/find/test/findModel.test.ts", l195
[ ] "./vs/editor/contrib/suggest/test/suggestMemory.test.ts", PR Open Here #66754,
[x] "./vs/workbench/parts/search/test/common/searchModel.test.ts", l680
[x] "./vs/workbench/parts/search/test/common/searchResult.test.ts", l681
[x] "./vs/workbench/parts/tasks/test/electron-browser/configuration.test.ts", l716
[x] "./vs/workbench/services/files/node/watcher/unix/test/chockidarWatcherService.test.ts", l803
[x] "./vs/workbench/services/search/test/node/search.test.ts", l869

@mjbvz mjbvz modified the milestones: On Deck, December/January 2019 Jan 29, 2019

@mjbvz

This comment has been minimized.

Copy link
Contributor Author

mjbvz commented Jan 29, 2019

Thanks everyone for your help! Thanks to your PRs, we now are now a good deal closer to being able to enable strict null checking for the entire VS Code codebase

@mjbvz mjbvz closed this Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment