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

test: Add test for "needCheckForUpdates" #1145

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

tanishiking
Copy link
Member

#1144

This commit adds a test for needCheckForUpdates.

For adding unit tests, this commit abstract away the logic that needs
access to vscode APIs, becuase we need to run an integration test for
accessing vscode APIs.
see: microsoft/vscode-wordcount#5 (comment)
microsoft/vscode#82471 (comment)

scalameta#1144

This commit adds a test for `needCheckForUpdates`.

For adding unit tests, this commit abstract away the logic that needs
access to vscode APIs, becuase we need to run an integration test for
accessing vscode APIs.
see: microsoft/vscode-wordcount#5 (comment)
microsoft/vscode#82471 (comment)
@tanishiking
Copy link
Member Author

Copied from tanishiking#82

kpodsiad commented 7 hours ago
One problem with assert is that it doesn't provide meaningful error messages. I was thinking about using earl, wdyt?

Yeah, adding an assertion library is good 👍
However, in that case, I would choose chai as its tried and tested solution (or I would pick jest instead of mocha + earljs as all-in-one test framework, but I personally prefer mocha + chai + sinon for testing node project).

repo
);
expect(actual).false;
expect(spy.getCall(0).args).to.eql([
Copy link
Member Author

Choose a reason for hiding this comment

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

we can use spy.calledWith(...), but this way will show better error message if test fail (while calledWith shows true or false)

@kpodsiad
Copy link
Member

kpodsiad commented Sep 2, 2022

Copied from tanishiking#82

kpodsiad commented 7 hours ago
One problem with assert is that it doesn't provide meaningful error messages. I was thinking about using earl, wdyt?

Yeah, adding an assertion library is good 👍 However, in that case, I would choose chai as its tried and tested solution (or I would pick jest instead of mocha + earljs as all-in-one test framework, but I personally prefer mocha + chai + sinon for testing node project).

Actually I was wrong. assert is good enough, it has meaningful errors and it doesn't require additional dependency. I intentionally switched jest to mocha in this repository simple because it's much smaller than jest and it doesn't bring so many dependencies (I recall some problems with them)

@tanishiking
Copy link
Member Author

I intentionally switched jest to mocha in this repository simple because it's much smaller than jest and it doesn't bring so many dependencies

Yeah, I also think mocha chai sinon would be noice in this repository (than jest) 👍
You don't want to add chai as a dependency? I prefer to use chai because I like BDD style testing and it's small library.

Copy link
Member

@kpodsiad kpodsiad left a comment

Choose a reason for hiding this comment

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

You don't want to add chai as a dependency? I prefer to use chai because I like BDD style testing and it's small library.

It's ok, I just meant that I forgot that node's assert also provides good errors ;)

I think we're missing one important think (correct me if I' worng) - do we have some docs how to run single test file, single test from suite? That would be nice to have.
TBH, even yarn test might be useful for people like don't do js day to day.

import { ExtensionContext, Memento } from "vscode";
import { ConfigurationTarget } from "../ConfigurationTarget";

type LastUpdated = {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's more common to use interface for such data types

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion, but I prefer to use type over interface for the data types that shouldn't be extended (by inheritance or declaration merging).

Comment on lines +9 to +18
export interface CheckForUpdateRepo {
getLastUpdated(target: ConfigurationTarget): LastUpdated;
saveLastUpdated(
serverVersion: string,
lastUpdatedAt: string,
target: ConfigurationTarget
): void;
}

export class DefaultCheckForUpdateRepo implements CheckForUpdateRepo {
Copy link
Member

Choose a reason for hiding this comment

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

SO it's not a problem that interface is located in the same file which has imports from vscode package as long as it doesn't use them, nice. Maybe I'll rewrite/write some unit tests for test explorer then :D

@tanishiking
Copy link
Member Author

Thanks! Added docs on how to run tests b70d5b3
merging after CIs pass

@tanishiking tanishiking merged commit 6f1d6bd into scalameta:main Sep 5, 2022
@tanishiking tanishiking deleted the test-need-check-update branch September 5, 2022 14:28
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