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

Remove regex check for test files #2074

Closed
bdwain opened this issue Jan 26, 2018 · 12 comments
Closed

Remove regex check for test files #2074

bdwain opened this issue Jan 26, 2018 · 12 comments
Assignees
Labels
AREA: server !IMPORTANT! STATE: Auto-locked An issue has been automatically locked by the Lock bot. SYSTEM: runner TYPE: enhancement The accepted proposal for future implementation.

Comments

@bdwain
Copy link
Contributor

bdwain commented Jan 26, 2018

Are you requesting a feature or reporting a bug?

request a feature

What is the current behavior?

There are regexes that each test file is run against, and if they do not match, you get an error

No tests to run. Either the test files contain no tests or the filter function is too restrictive.

What is the desired behavior?

Can this regex test be disabled and instead look for tests when actually running/parsing the code? I've found valid use cases where those regexes might not match.

In my project, I've broken up my app into several "subapp" libraries, and each subapp exports a suite of tests, which are run in the main repo's testcafe suite.

for example:

library test file

export default function libraryTests(){
  fixture('Library tests').page('localhost:1234');

  test('Does some stuff', async t => {
    //await t.expect(....); //expect something
  });
}

main test file

import {libraryTest} from 'library';

libraryTest();

currently, this fails when i try to run the tests pointing at the main test file. However, it works if i add a commented out fixture and test. Ideally, I would not need to add a commented out fixture and test just to satisfy the regex.

@kirovboris
Copy link
Collaborator

Hi @bdwain,

In your example, simple parsing of the main test file won't find the fixture as well.
Besides, it'll be much slower than regexp. If we start parsing all dependencies, it can require much time, because it should also parse all your modules/libraries/node_modules... which were imported in the test file.

We can't get fixture/test names during script execution, because it can cause side effects. E.g. you have a .js file to setup test database script in a directory with tests. We can't execute it to check if it's a fixture file. So we need static file analysis anyway.

@bdwain
Copy link
Contributor Author

bdwain commented Jan 26, 2018 via email

@kirovboris
Copy link
Collaborator

It seems like it’s only used to filter out source files that have no tests and fixtures.

Yes, it's correct.

But you could just assume there are tests in each file and error if there are not.

As I said before, we can't run every file by glob patterns like test/**/*.js because of side effects in non test files.
For now, we suppose a user doesn't have to concern whether the passed files have code with side effects or not, e.g. you can just run

testcafe chrome tests/**/*.js

instead of

testcafe chrome tests/e2e/fixtures/**/*.js

It can matter if you're going to add testcafe tests to an existing project with a complex directory structure.
If you need a single entry point for tests in your libraries, you can implement a nodejs runner using TestCafe programming API and get rid of export statements in your fixtures.

@bdwain
Copy link
Contributor Author

bdwain commented Jan 29, 2018

I am already using the node api. Could the regex check be disabled with an option then? This way it's a new behavior that only affects people who opt-in. something like

runner.run({runAllFiles: true})

@kirovboris
Copy link
Collaborator

Unfortunately, we wouldn't like to add an option that can cause unexpected effects on a user's computer. As a workaround, you can regroup tests or change a directory structure.

@bdwain
Copy link
Contributor Author

bdwain commented Mar 26, 2018

@kirovboris It would not be an unexpected effect. The entire point of the option is to run every file in the user's regex, so if they enable it, they should know what it does (and it would presumably be explained in the docs).

This would prevent needing a hacky workaround to share suites across different files, so I really think it's something that should be addressed.

@EvanNGoldstein
Copy link

Any update on this? In order to share suites across files I have to add something like

fixture `A fixture`;

test('', () => {});

to my files, which is extremely hack-y...

@bxt
Copy link

bxt commented Jul 10, 2018

I also think these regexes should be removed for two reasons:

  • Like mentioned before it prevents you from putting test case generators into a library file. The tests will just stop running without any notice. This left me wondering with no indication what happended.
  • Also if you do rely on the behavior of the regexes and have e.g. some database cleanup code in a JS file, once a developer puts in test. and fixture. into them (even in a comment) they are suddenly run. This is I think very unexpected and might cause data loss etc.

I also do not agree with @kirovboris's previous points:

As I said before, we can't run every file by glob patterns like test/**/*.js because of side effects in non test files.

Most people probably name their tests files something like foo.spec.js anyways, so one could use test/**/*.spec.js then. There are also other tools which need extra configuration just for testcafe (see e.g. #1735) and those also use such a globs, so there would be no changes required in most cases.

For now I will put a comment like this into every test file:

// test( fixture( <- workaround for https://github.com/DevExpress/testcafe/issues/2074

Which unlike Evan's solution does not require any actual code but still is very hack-y.

@bdwain
Copy link
Contributor Author

bdwain commented Sep 17, 2018

agreed with @bxt . bump. This issue really should be addressed.

@kirovboris
Copy link
Collaborator

Hi @bdwain @bxt,

I agree that the current implementation can cause problems with test generators.
We decided to add an option that enables the behavior you described in the upcoming releases.
Later, when we release a major update, we will enable this behavior by default.

@kirovboris kirovboris added this to the 1.0.0 Breaking Changes milestone Sep 20, 2018
@bdwain
Copy link
Contributor Author

bdwain commented Sep 23, 2018

awesome! thanks!

@AndreyBelym AndreyBelym added TYPE: enhancement The accepted proposal for future implementation. SYSTEM: runner AREA: server and removed TYPE: proposal labels Oct 1, 2018
@AndreyBelym AndreyBelym modified the milestones: 1.0.0 Breaking Changes, Sprint #19 Oct 1, 2018
@AndreyBelym AndreyBelym modified the milestones: Sprint #19, Sprint #20 Oct 15, 2018
@AlexKamaev AlexKamaev self-assigned this Oct 17, 2018
@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Feb 21, 2019
@AlexSkorkin AlexSkorkin removed the STATE: Need response An issue that requires a response or attention from the team. label Feb 26, 2019
@lock
Copy link

lock bot commented Mar 27, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or feature requests. For TestCafe API, usage and configuration inquiries, we recommend asking them on StackOverflow.

@lock lock bot added the STATE: Auto-locked An issue has been automatically locked by the Lock bot. label Mar 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2019
kirovboris pushed a commit to kirovboris/testcafe-phoenix that referenced this issue Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
AREA: server !IMPORTANT! STATE: Auto-locked An issue has been automatically locked by the Lock bot. SYSTEM: runner TYPE: enhancement The accepted proposal for future implementation.
Projects
None yet
Development

No branches or pull requests

9 participants