-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Automatically remove directories of missing tests #2009
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be quite a bit of noise in the code style here which may be worth checking is intended?
Otherwise will be nice to have this - also had to blast a rm -r test && git checkout test
a few times here.
test/utils.js
Outdated
return require( configFile ); | ||
} catch ( err ) { | ||
const dir = path.dirname( configFile ); | ||
console.warn( `Failed to load ${configFile}.\nRemoving directory ${dir} as this is probably the result of a missing test.` ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A syntax error in a configuration file may be a mistake when editing _config.js and shouldn't result in removing the folder... perhaps we can filter specifically to an ENOENT error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Now the directory is only removed for a MODULE_NOT_FOUND error (which is the ENOENT for require
).
5b66d13
to
fbad78a
Compare
I removed the noise in the output. The issue was that the test files were rather different from our usual formatting and I just let my IDE auto-format these files. But I guess it makes more sense to wait until we have prettier to handle these things once and for all. |
@guybedford ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny how the smallest changes can have the most feedback :P
test/utils.js
Outdated
if ( err.code === 'MODULE_NOT_FOUND' ) { | ||
const dir = path.dirname( configFile ); | ||
console.warn( `Test configuration ${configFile} not found.\nRemoving directory as this test probably no longer exists.` ); | ||
sander.rimrafSync( dir ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to be annoying, but I'm still just picturing cases where someone might write a test without realising they need to create a _config.js file, then watch as their carefully crafted test gets deleted... maybe we should make this even stricter and just do a straightforward rmdir
that is allowed to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's now as safe as it can be to still cover the specific use case. The "_actual" folder is removed via rimraf
as this is what is not cleaned up by git, then we try to remove the test dir via rmdir
which will fail if there are other files present.
@@ -15,7 +15,7 @@ describe('chunking form', () => { | |||
|
|||
const config = loadConfig(samples + '/' + dir + '/_config.js'); | |||
|
|||
if (config.skipIfWindows && process.platform === 'win32') return; | |||
if ( !config || (config.skipIfWindows && process.platform === 'win32') ) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only worry now is that we are potentially mixing code styles. I know we stopped the spacing convention with the TS conversion, but if we want to maintain it for the JS and tests that's fine, but we should at least be consistent I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I do not really care about one formatting or the other. Formatting can easily be changed on way or another instantly via software and I think that is ultimately the way we should handle it to no longer waste time with this. If #1896 is advanced, it should be easy to let it cover some of the .js files as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will leave the formatting as it is to keep the diff smaller. No use trying to copy the arbitrary style of the rest of the file IMO though.
Looks safe now to me! |
Cool! I'll just put this into master without another release as this is not a user-facing change anyway. |
This removes a common nuisance when switching between different branches. Previously when there was no
_config.js
file in a test folder, an error would be thrown and the tests would not execute at all until the folder in question was removed.As in 100% of the cases I experienced this folder could always be safely removed, this will now just display a warning and remove the folder in question. My hope is that if this should ever misfire (due to e.g. larger refactorings of the test system), we will always have the git history to rely on.