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

next {dev,build} command should ignore __tests__ dir #1914

Closed
brikou opened this issue May 7, 2017 · 12 comments
Closed

next {dev,build} command should ignore __tests__ dir #1914

brikou opened this issue May 7, 2017 · 12 comments

Comments

@brikou
Copy link
Contributor

brikou commented May 7, 2017

In jest example, if we move ./__tests__ to ./pages/__tests__ and then run next build then an error occurs.

The solution is not trying to solve this error, but make next ignore some patterns, fe *.{spec,test}.js or some dirs like __{mocks,tests}__

Otherwise, those dev files would be available in public dir, WDYT ?

Issues #988 #1545 #1689 talks about the same problem (sadly without any solution).

BTW next.js rocks!

@bensalilijames
Copy link
Contributor

Design suggestion: add an excludeFile option to next.config.js which accepts a function to exclude certain files from being webpack'd.

This option would be passed straight to emit-file-loader's exclude option (and merged with the normal node_modules exclude) to prevent it from building it as a page. For example:

// next.config.js
module.exports = {
  excludeFile: (str) => /\*.{spec,test}.js/.test(str)
}

If that sounds good I can make a PR for this!

@arunoda
Copy link
Contributor

arunoda commented May 31, 2017

I don't like to take this into core at the moment.

Try to change you build script to delete these files after the build for now.

@arunoda arunoda closed this as completed May 31, 2017
@sboudrias-aa
Copy link

@arunoda I think you should reconsider this. Testing is very important, and right now this forces users to creates unnecessary test directories mapping the file structure elsewhere.

If there was an easy way to manually update next config to ignore test files it wouldn't be a problem, but I don't see any easy way to do this at the moment.

In that case, the developers are really left out without options. How are you organizing tests internally at next? I can't believe you're not running into this issue..?

@sboudrias-aa
Copy link

Might be able to simply add a ignore plugin to next.config.js

config.plugins.push(new webpack.IgnorePlugin(/pages.*\/test.*/));

@novascreen
Copy link
Contributor

this would be nice, I think a lot of people are putting their tests next to their components, rather than having a single tests folder outside of the app recreating the app's folder structure

@ericnograles
Copy link

ericnograles commented Sep 5, 2017

Hey folks,

Thought I'd chime in here with my approach. As @sboudrias-aa indicated, you'll just need to override the next.config.js with the ignore-loader. You'll obviously need to add ignore-loader as a dependency.

Here's what my next.config.js looks like, ignoring any file with a .test.js pattern:

const path = require('path');
const glob = require('glob');

module.exports = {
  webpack: (config, { dev }) => {
    config.module.rules.push(
      {
        test: /\.test.js$/,
        loader: 'ignore-loader'
      }
    );
    return config;
  }
};

@SBoudrias
Copy link
Contributor

@ericnograles an issue we ran into down the road though is that this was throwing off the common module bundling. The rule only add a module to the common bundle if > 2/3 of pages uses it; and the calculation counts the tests files into that (even though you ignore them with a loader)

@ericnograles
Copy link

Thanks for the tip @SBoudrias I'll keep an eye on that.

So, it appears that we ought not fight the conventions of Next.js and just store our tests on a top level __tests__ folder. Darn. Hope there's a workaround for this in the future, as side-by-side test files really help reduce the overall annoyance of unit testing for my team.

@scsper
Copy link

scsper commented Sep 6, 2017

agreed with @sboudrias-aa and @ericnograles . @arunoda please reconsider this decision. writing tests next to the module also helps a lot with refactoring because you can delete / move entire directories and still have things work.

@LightningK0ala
Copy link

Voicing my support for @arunoda to reconsider. Co-located tests is also a preference for us. @benhjames suggestion looks clean!

@robhowell
Copy link

@arunoda Test colocation makes a massive difference to the developer experience, which is why more projects are moving to that model of file organisation. From my experience, I've found it to be a very beneficial pattern on React projects - unit tests tend to be much better maintained by a team when they are colocated with the source files, I recommend trying it out!

  1. Is there a specific technical concern about the change suggested by @benhjames, e.g. that it will overcomplicate the Webpack build?

  2. If you could tell us why you are unwilling to allow that change, I'm sure that there are enough developers that want this feature (such as myself) that would be happy to investigate alternative solutions - if we did that would you be willing to consider them?

Thanks for all your work on Next.js btw, I'm also a big fan of React Storybook!

@lock
Copy link

lock bot commented May 10, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants