Skip to content
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

Combine test compile step into normal compile #5666

Merged
merged 2 commits into from
May 4, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented May 3, 2023

This makes it so compile also compiles tests. There were lots of nullable warnings which I either fixed or suppressed when makes sense.

Unfortunately it looks like chai/mocha methods don't tell the TS compiler when things have been asserted to be non-null, so there are lots of !.

@dibarbet dibarbet force-pushed the dev/dibarbet/compile_tests branch 4 times, most recently from a0d05d3 to dd03af8 Compare May 3, 2023 02:16
@dibarbet dibarbet force-pushed the dev/dibarbet/compile_tests branch from dd03af8 to ed2899a Compare May 3, 2023 18:42
@dibarbet dibarbet marked this pull request as ready for review May 3, 2023 20:10
@dibarbet dibarbet requested review from JoeRobich and 333fred May 3, 2023 20:10
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for taking on the job of fixing up the tests. Had a few suggestions but I would be fine if things merged as is.

@@ -31,66 +31,66 @@ suite("GetOmnisharpPackage : Output package depends on the input package and oth

test(`Architectures, binaries and platforms do not change useFramework: ${useFramework}`, () => {
let testPackage = inputPackages.find(element => (element.platformId && element.platformId == "os-architecture"));
let resultPackage = SetBinaryAndGetPackage(testPackage, useFramework, serverUrl, version, installPath);
let resultPackage = SetBinaryAndGetPackage(testPackage!, useFramework, serverUrl, version, installPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a helper such as the one in this TS playground, then the compiler would be happy.

function isNotNull<T>(value: T) : asserts value is NonNullable<T> {
    if (value === null || value === undefined) {
        throw new Error("value is null or undefined.");
    }
}

Copy link
Member Author

@dibarbet dibarbet May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this, and updated all the places that I thought made sense to use it

  1. Places that were asserting not null using the non-annotated APIs were replaced with this function
  2. Places where the nullable item was used multiple times (and would have required multiple !) I also replaced

Otherwise, I left the ! usage when the nullable item was used only once

tsconfig.json Show resolved Hide resolved
@dibarbet dibarbet force-pushed the dev/dibarbet/compile_tests branch from 7abd1d7 to 36f342b Compare May 4, 2023 18:27
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes look good!

@dibarbet dibarbet merged commit f267b16 into dotnet:master May 4, 2023
@dibarbet dibarbet deleted the dev/dibarbet/compile_tests branch May 4, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants