Skip to content

Commit

Permalink
Normalize the name of the function used
Browse files Browse the repository at this point in the history
  • Loading branch information
@oliviertassinari
oliviertassinari committed Jan 9, 2021
1 parent 4ec9daf commit ab0df30
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion packages/material-ui/src/MenuItem/MenuItem.test.js
View file
Expand Up @@ -115,7 +115,7 @@ describe('<MenuItem />', () => {
expect(handleKeyDown.callCount).to.equal(1);
});

it('should fire onTouchStart', function touchStartTest() {
it('should fire onTouchStart', function test() {

This comment has been minimized.

Sign in to view

Copy link
@eps1lon

eps1lon Jan 9, 2021

Member

These are useful for debugging callstacks. Please refrain from consistency fixes in the future if they don't serve any purpose

This comment has been minimized.

Sign in to view

Copy link
@oliviertassinari

oliviertassinari Jan 10, 2021 •
edited

Author Member

The objective is to make creating new tests easier: removing the need to rename the function when using a copy & paste.

What's the use case for debugging the callstack here? Most of the time, we are using anonymous arrow functions, is it any different? (I don't understand when this is useful)

This comment has been minimized.

Sign in to view

Copy link
@eps1lon

eps1lon Jan 10, 2021 •
edited

Member

(I don't understand when this is useful)

And at this point you should ask the author instead of just changing the code. You cannot assume that you know better.

What's the use case for debugging the callstack here?

This should give a good example: jestjs/jest#10099. It also includes links to more in-depth resources that should help you understand the problem.

Otherwise I'm assuming you've never used the debugger for test which is fine. Just don't assume that everybody works like you.

The objective is to make creating new tests easier: removing the need to rename the function when using a copy & paste.

If changing the function name when copying a test is the biggest problem, we have different problems to solve first. Did you ever encounter this problem or did you just make this up?

This comment has been minimized.

Sign in to view

Copy link
@oliviertassinari

oliviertassinari Jan 11, 2021 •
edited

Author Member

I'm assuming you've never used the debugger

True, I have never trusted it or myself to use it correctly. I have forced a habit to use the console. Maybe if I used the debugger more I would find myself more efficient with it, I'm far from it yet

jestjs/jest#10099

The solution proposed inside it looks great, it's weird to have to name the test and the function, twice.

Did you ever encounter this problem or did you just make this up?

We often face this problem in the documentation when developers are creating new demos from an existing one. They forget to rename the function to match the filename. The more I see this problem reproduce, the more I think that all the demos should be called "function Demo() {". It's probably good enough.

I was expecting this to be a mistake that slept through the reviews. It's not the case. I have reverted

This comment has been minimized.

Sign in to view

Copy link
@eps1lon

eps1lon Jan 12, 2021

Member

So you confirm that you just made this up. You never saw anybody face this problem.

This comment has been minimized.

Sign in to view

Copy link
@eps1lon

eps1lon Jan 12, 2021

Member

If you think the copied demo name is a problem why didn't you propose that every demo component is named Demo? Instead you identify a problem in A but apply the solution to B instead of applying the solution to where you've identified the problem.

This comment has been minimized.

Sign in to view

Copy link
@oliviertassinari

oliviertassinari Jan 12, 2021 •
edited

Author Member

If you think the copied demo name is a problem why didn't you propose that every demo component is named Demo?

I have mentioned the idea twice in the past on random PRs. I have never done a proper RFC for it because I never felt the pain was strong enough compared to the cost of renaming all the existing demos.
What do you think about it?
// only run in supported browsers
if (typeof Touch === 'undefined') {
this.skip();
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/StepButton/StepButton.test.js
View file
Expand Up @@ -71,7 +71,7 @@ describe('<StepButton />', () => {
});

describe('event handlers', () => {
it('should forward mouseenter, mouseleave and touchstart', function touchTests() {
it('should forward mouseenter, mouseleave and touchstart', function test() {
// only run in supported browsers
if (typeof Touch === 'undefined') {
this.skip();
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/test/integration/MenuList.test.js
View file
Expand Up @@ -558,7 +558,7 @@ describe('<MenuList> integration', () => {
}, 500);
});

it('should match ignoring hidden text', function testHiddenText() {
it('should match ignoring hidden text', function test() {
if (!innerTextSupported) {
// Will only be executed in Karma tests, since jsdom doesn't support innerText
this.skip();
Expand Down

0 comments on commit ab0df30

Please sign in to comment.