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

lowercase-name rule ignoreTopLevelTest option #612

Closed
overlookmotel opened this issue Jun 23, 2020 · 4 comments
Closed

lowercase-name rule ignoreTopLevelTest option #612

overlookmotel opened this issue Jun 23, 2020 · 4 comments

Comments

@overlookmotel
Copy link

overlookmotel commented Jun 23, 2020

To complement the ignoreTopLevelDescribe option added in #611, would it be possible to also have an ignoreTopLevelTest option?

I'd envisage it affecting both it and test - I doubt many people use both it and test in the same file, so it seems excessive to have separate ignoreTopLevelTest and ignoreTopLevelIt options.

An additional option ignoreTopLevel could also set both ignoreTopLevelDescribe and ignoreTopLevelTest.

Does this sound like a feature you'd accept? If so, I'd be happy to do a PR based on #611.

@overlookmotel overlookmotel changed the title ignoreTopLevelTest option lowercase-name rule ignoreTopLevelTest option Jun 23, 2020
@G-Rath
Copy link
Collaborator

G-Rath commented Jun 23, 2020

I don't feel like this makes sense, as the whole idea behind lowercase-name is about encouraging sentences that make grammatical sense; the reason we have ignoreTopLevelDescribe is because describe blocks "describe" things, and things like classes & react components start with a capital letter.

However, test & it are the start of a sentence, and so there's no common grammatical correct sentence that has the second word starting with a capital, except maybe test('MyClass to ensure it does the thing'), which I feel would be better wrapped in a describe anyway.

@overlookmotel
Copy link
Author

overlookmotel commented Jun 23, 2020

I guess it depends on how you write you test "sentences". I typically test that a library's exports are exported as intended, with something like this:

import { MyClass } from 'my-library';

test( 'MyClass is a function', () => {
  expect( typeof MyClass ).toBe('function');
} );

Does that seem like a reasonable case to you?

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 23, 2020

That makes sense, but I'm still lukewarm on adding a new option for this as I feel it leads to adding more options that are starting to miss the point of the rule.

In your case, you can make lowercase-name happy by just starting with that:

import { MyClass } from 'my-library';

test( 'that MyClass is a function', () => {
  expect( typeof MyClass ).toBe('function');
} );

describe is considered an exception for this because it's a lot harder to come up with a good, grammatical correct title that is not long winded for wrapping things whose names are in PascelCase; i.e:

describe('MyClass', () => {
  describe('#myMethod', () => {
    it('returns the right value', () =>{
      // ...
    });
  });
});

In the above, there isn't really a nice way to structure the title to have a lowercase name that reads correctly and doesn't require using a different structure to other tests for things like functions:

  • "[describe] that MyClass",
  • "[describe] the class",
  • "[describe] the methods in MyClass"

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 13, 2020

I'm going to close this off due to the reasons outlined in my previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants