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

Add tests for the deprecations API #1967

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Add tests for the deprecations API #1967

merged 3 commits into from
Apr 3, 2024

Conversation

jathak
Copy link
Member

@jathak jathak commented Apr 1, 2024

See sass/sass#3826

[skip dart-sass]
[skip sass-embedded]

});

describe('deprecation flags', () => {
it('warning emitted with no flags', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: test case descriptions should generally form sentence fragments that describe the subject of describe() (that's why the test function is called it()—so you can read the whole thing as a sentence). So for example, you might write:

describe('a warning', () => {
  it('is emitted with no flags', ...);
  it('is emitted with different deprecation type silenced', ...);
  // ...
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Also restructure test names to read as sentences
@jathak jathak requested a review from nex3 April 2, 2024 23:52
const extraDeprecations = Object.keys(deprecations).filter(
deprecation => !expectedDeprecations.includes(deprecation)
);
expect(extraDeprecations).toBeEmptyArray();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check that everything in expectedDeprecations is in deprecations as well. Also, we should probably have some sort of test for the deprecation list in the JS API spec as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an additional check that there are no missing deprecations either.

I also added some types which should ensure that there aren't any extra deprecations in these tests that aren't in the spec.

We now check that there are no missing deprecation types and also use
types to ensure that every deprecation in the tests is also in the spec.
@jathak jathak merged commit 3aae845 into main Apr 3, 2024
7 checks passed
@jathak jathak deleted the deprecations branch April 3, 2024 21:14
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.

None yet

2 participants