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

Improving error messages and codes in jest-console, matcher.js #53743

Merged

Conversation

MericKarabulut
Copy link
Contributor

What?

This pull request addresses issue #17072, it improves error messages and enhanges code maintainability.

Why?

This fix is importantbecause jest-console gives confusing errors when developers use console messages for their testes. These unclear error messages can easily confuse developers and make them think that they've set up their testing tools wrong or that something is broken in their tests.

How?

I added "console.log() should not be used unless explicitly expected, see file://gutenberg/packages/jest-console/README.md."(using methodName parameter so it works with all console metods) and I contributed by introducing functions like createErrorMessage and createSpyInfo, for setting the stage for possible future improvements to this file.

Testing Instructions

  1. you can open a example.test.js file
  2. write a sipmple test with console outputs.
  3. test your sample test with npm run test:unit ./example.test.js
  4. change console inputs, console metods etc. and try possibilities

Testing Instructions for Keyboard

none

Screenshots or screencast

none

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 16, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @MericKarabulut! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Package] Jest console /packages/jest-console labels Aug 16, 2023
@gziolo
Copy link
Member

gziolo commented Aug 17, 2023

Thank you for starting this PR. It looks like some tests using jest-console fail now. It’s very likely that the matcher needs to ignore the newly introduced part of the information used when performing assertions.

@MericKarabulut
Copy link
Contributor Author

pls do not merge yet i spotted the problem. The problem accurs when test use toHaveLoggedWith("some expected log") it try to compare calls with expected, now it always returns false to pass parameter. so i'm debugging now i tryed with calls.some((objects) => objects.includes(expected) and many others but none of my solutions seems to be working. original code was const pass = calls.some( ( objects ) =>this.equals( objects, expected )); and i cant pass this keyword to my createSpyInfo function also it refers to jest util so i need to figure it out. i will update pr as soon as possiable. It's my bad 😊

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

The fact that code get refactored together with the new changes added makes it a bit harder to review. What's the most important part I should be looking at exactly?

packages/jest-console/src/matchers.js Outdated Show resolved Hide resolved
packages/jest-console/src/matchers.js Outdated Show resolved Hide resolved
@MericKarabulut
Copy link
Contributor Author

MericKarabulut commented Aug 28, 2023

The fact that code get refactored together with the new changes added makes it a bit harder to review. What's the most important part I should be looking at exactly?

Yeah you are right, thank you for the review. During the code addition phase, I retained the original message and included a reference in the documentation. For the modifications in the displayed message, you can refer to the createErrorMessage function. In the refactoring part, I combined the messaging creation and console input control functionalities among custom matchers into createSpyInfo function. I believed that this would enhance maintainability and debugging. Moreover I thought it made sense to design the createSpyInfo function to gather potential shared features for custom matchers in the future.

The most important aspect to note was ensuring that the original functionality of matchers remained intact and performed with the same efficiency. As using Jest's built-in object comparison mechanism within the spyInfo function was not possible. Instead,I had to adopt an alternative approach for comparison replacingthis.equals with JSON.stringify. I believe this change would have a positive impact on performance, However, it's important to be conscious.
Thanks for the review again. I'll add your suggestions and update the pull request

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Excellent work on this PR. I have tested it with the custom code added:

	console.log( true, null );
	console.warn( '123', 'ccc' );
	console.error( [ '123' ], { a: 'ccc' } );

It all works as expected:

Screenshot 2023-08-30 at 11 16 20 Screenshot 2023-08-30 at 11 16 29 Screenshot 2023-08-30 at 11 16 36

All the existing unit tests for @wordpress/jest-console still pass.

I left a super tiny comment, which is only to improve readability. I would appreciate also adding an entry to the CHANGELOG file:

https://github.com/WordPress/gutenberg/blob/trunk/packages/jest-console/CHANGELOG.md

along these lines:

### Enhancement

- Improved error messages and codes printed on the console ([#53743](https://github.com/WordPress/gutenberg/pull/53743)).

By the way, thank you so much for the detailed explanation of the changes applied. In retrospect, I think it would help if the commits would get split into two steps where the refactoring would be an atomic commit. No need to change anything now, but I thought I would share that.

packages/jest-console/src/matchers.js Show resolved Hide resolved
@MericKarabulut
Copy link
Contributor Author

Changes added 👍, It was a pleasure to contribute. In my future contributions, I'll make sure to separate my commits in a clear manner. Thank you for review and your guidance. 🙏

@gziolo gziolo enabled auto-merge (squash) August 30, 2023 22:09
@gziolo gziolo merged commit 5410389 into WordPress:trunk Aug 30, 2023
49 checks passed
@github-actions
Copy link

Congratulations on your first merged pull request, @MericKarabulut! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Aug 30, 2023
@bph bph added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 19, 2023
@github-actions
Copy link

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Type] Enhancement, [Type] Code Quality, [Package] Jest console, First-time Contributor.

Read more about Type labels in Gutenberg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Jest console /packages/jest-console [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants