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

32129139: Try jest on yaem #162

Merged
merged 4 commits into from
Apr 9, 2024
Merged

32129139: Try jest on yaem #162

merged 4 commits into from
Apr 9, 2024

Conversation

DamienCassou
Copy link
Member

@DamienCassou DamienCassou commented Apr 3, 2024

This change is Reviewable

@DamienCassou DamienCassou force-pushed the 32129139/Try_jest_on_yaem branch 2 times, most recently from 57f33ca to dfdf35e Compare April 3, 2024 11:54
@DamienCassou DamienCassou marked this pull request as ready for review April 3, 2024 11:57
Copy link
Contributor

@BenjaminVanRyseghem BenjaminVanRyseghem left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamienCassou, @danglotb, @JohanBriglia, @josquindebaz, and @ValentinaVasile)


jest.config.mjs line 6 at r1 (raw file):

 */

/** @type {import('jest').Config} */

nit: do we need this line?

Copy link
Contributor

@josquindebaz josquindebaz left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamienCassou, @danglotb, @JohanBriglia, and @ValentinaVasile)


-- commits line 13 at r3:
🥳

Copy link

@JohanBriglia JohanBriglia left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamienCassou, @danglotb, and @ValentinaVasile)


-- commits line 13 at r3:

Previously, josquindebaz (Josquin Debaz) wrote…

🥳

Did you already check how much code coverage we have? 😁

Copy link
Member Author

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @danglotb, and @ValentinaVasile)


-- commits line 13 at r3:

Previously, JohanBriglia wrote…

Did you already check how much code coverage we have? 😁

image.png

You can check that yourself in the CI results.


jest.config.mjs line 6 at r1 (raw file):

Previously, BenjaminVanRyseghem (Benjamin Van Ryseghem) wrote…

nit: do we need this line?

We don't need it but it's going to be convenient for anyone using LSP (including VS code and Emacs): it provides completion and documentation.

image.png

image copy 1.png

@DamienCassou DamienCassou force-pushed the 32129139/Try_jest_on_yaem branch 5 times, most recently from 089b3df to db09188 Compare April 5, 2024 12:39
Copy link
Contributor

@BenjaminVanRyseghem BenjaminVanRyseghem left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DamienCassou, @danglotb, and @ValentinaVasile)


jest.config.mjs line 6 at r1 (raw file):

Previously, DamienCassou (Damien Cassou) wrote…

We don't need it but it's going to be convenient for anyone using LSP (including VS code and Emacs): it provides completion and documentation.

image.png

image copy 1.png

ok, thanks

@DamienCassou DamienCassou force-pushed the 32129139/Try_jest_on_yaem branch from db09188 to 711990f Compare April 8, 2024 10:06
@DamienCassou DamienCassou force-pushed the 32129139/Try_jest_on_yaem branch from 711990f to 8ff795c Compare April 9, 2024 10:24
Copy link
Contributor

@BenjaminVanRyseghem BenjaminVanRyseghem left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r6, 1 of 1 files at r7, 9 of 9 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @DamienCassou, @danglotb, and @ValentinaVasile)


babel.config.cjs line 17 at r4 (raw file):

				test: {
					plugins: [
						"rewire",

nit: why removing rewire?


test/events.test.js line 109 at r9 (raw file):

	});

	it("un-Bind callback using unbind", () => {

nit: unbinds?


test/events.test.js line 175 at r9 (raw file):

	});

	it("event Category can bind callback to named event using register", () => {

nit: shouldn't tests start with a verb?

Copy link
Member Author

@DamienCassou DamienCassou left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @BenjaminVanRyseghem, @danglotb, and @ValentinaVasile)


babel.config.cjs line 17 at r4 (raw file):

Previously, BenjaminVanRyseghem (Benjamin Van Ryseghem) wrote…

nit: why removing rewire?

it's not necessary to make the tests pass ;-). I have nothing against adding it back later but I'm keeping the stack as small as possible for now


test/events.test.js line 109 at r9 (raw file):

Previously, BenjaminVanRyseghem (Benjamin Van Ryseghem) wrote…

nit: unbinds?

these changes were done automatically by eslint plugins (not yet enabled in this PR because I have a dedicated one in dev-doc). I agree more changes are necessary to make the code follow the guideline. It feels out of scope though.


test/events.test.js line 175 at r9 (raw file):

Previously, BenjaminVanRyseghem (Benjamin Van Ryseghem) wrote…

nit: shouldn't tests start with a verb?

definitely. In this case, I think we need a dedicated describe() block and maybe even a dedicated test file. This is out of scope of this PR though. This change was done automatically by eslint. https://github.com/jest-community/eslint-plugin-jest/blob/HEAD/docs/rules/prefer-lowercase-title.md

@DamienCassou DamienCassou merged commit 46ba8df into main Apr 9, 2024
1 of 2 checks passed
@DamienCassou DamienCassou deleted the 32129139/Try_jest_on_yaem branch April 9, 2024 14:20
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.

4 participants