Skip to content
This repository has been archived by the owner on Sep 4, 2023. It is now read-only.

Runner fails if an array of testPaths are given and the folders are missing #167

Closed
aminya opened this issue Oct 16, 2020 · 11 comments
Closed

Comments

@aminya
Copy link
Contributor

aminya commented Oct 16, 2020

Runner fails if an array of testPaths are given and the folders are missing. Here I gave two folders, but because the next one is missing the runner fails. There should be a check before erroring.

const { createRunner } = require("atom-jasmine3-test-runner")

// https://github.com/UziTech/atom-jasmine3-test-runner#api
module.exports = createRunner({
  testPackages: [],
  suffix: "-test",
  legacySuffix: "-test-v1",
  testPaths: ["__tests__", "__atom_tests__"],
  timeReporter: true,
  silentInstallation: true,
  specHelper: false
})
Error: ENOENT: no such file or directory 'C:\Users\aminy\Documents\GitHub\JavaScript\@atom-ide-community\atom-ide\nuclide\nuclide-adb\__atom_tests__' from testPaths
@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

I don't think failing silently is the right approach. If someone misspells a test path they should be alerted to the error instead of needing to debug why it is not finding any tests.

@aminya
Copy link
Contributor Author

aminya commented Oct 16, 2020

@UziTech A message must be enough. Jasmine itself (or any other test framework I am aware of) does not fail when it does not find any tests.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

Jasmine and jest both fail if no tests are found.

I think a better solution to the problem "Runner fails if a folder is missing" is to remove or create that folder.

Errors are meant to alert the user of something that is wrong to make debugging easier. That error seems to do that pretty well.

@aminya
Copy link
Contributor Author

aminya commented Oct 16, 2020

I made this optional defaulting to false to eliminate any debates 😄
#168

This is a must for a monorepo in which I want to set a general test runner and not configure it for every single package.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

This seems like an unnecessary option since the runner.js file is dynamic and testPaths can change based on where it is run from. This seems like an issue that should be solved in your repo not for everyone using atom-jasmine3-test-runner.

This is called feature creep.
image

@aminya
Copy link
Contributor Author

aminya commented Oct 16, 2020

Well, I think there are enough customers for this feature that we should consider it. Another potential customer is Atom itself which runs the tests for all of its packages. Many packages such as "language-*" do not have any tests, but the test runner skips those

https://github.visualstudio.com/Atom/_build/results?buildId=85175&view=logs&j=c5d9a21a-e43e-5eaa-4a5a-044feacfb7bd&t=635c38fb-254d-503b-5f5b-4154e4bec223&l=24

I think this and #166 were two of the reasons that because of whom atom-community/atom#57 was failing.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

This is why the options are sent in a .js file not a .json file. You could test if a folder exists yourself and remove it from testPaths if it does not.

This sort of dynamic config allows atom-jasmine3-test-runner to not have a lot of options.

Since this is something that can be solved without changing atom-jasmine3-test-runner I don't see any reason to change it.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

Many packages such as "language-*" do not have any tests

This issue is about a folder passed to testPaths not existing. PR #168 won't solve the issue about no test being run.

I think the solution for that problem should be to add tests, even if they are simple tests making sure they don't fail on activate.

@aminya
Copy link
Contributor Author

aminya commented Oct 16, 2020

This is why the options are sent in a .js file not a .json file. You could test if a folder exists yourself and remove it from testPaths if it does not.

This sort of dynamic config allows atom-jasmine3-test-runner to not have a lot of options.

Since this is something that can be solved without changing atom-jasmine3-test-runner I don't see any reason to change it.

This results in atom-jasmine3-test-runner-runner 😄. OK. That's fine. I can't create this package, but the name would be funny.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

You don't have to create aseparate package. You would change the code in https://github.com/atom-ide-community/atom-ide-base/pull/33/files#diff-6bc7bbee028394434de4b833d18636d86df3811726d16d0be0cb56446a4676acR8

Instead of sending the array (["__tests__", "__atom_tests__"]) you would check if the path exists first.

@UziTech
Copy link
Owner

UziTech commented Oct 16, 2020

atom-community/atom-ide-base#33 (review) is how you should fix this.

@UziTech UziTech closed this as completed Oct 16, 2020
aminya added a commit to atom-community/atom-ide-base that referenced this issue Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants