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

test: give each test suite an indepedent workspace #403

Merged
merged 6 commits into from
May 17, 2024

Conversation

sockmaster27
Copy link
Contributor

Gives each test-suite its own workspace in the test/testWorkspaces/ directory.
The contents of each workspace is then copied to the test/activeWorkspace/ directory at the appropriate time.

test/src/util.ts Outdated

// Delete all files except .gitkeep and flix.jar
const namesToDelete = names.filter(name => name !== '.gitkeep' && name !== 'flix.jar')
const urisToDelete = namesToDelete.map(name => vscode.Uri.joinPath(activeWorkspaceUri, name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am always very careful with file deletion. Would it be sensible to require the file name to end in .flix? Or at list of extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if the tests should be reliable the active workspace needs to be reset entirely, including flix.toml and all packages which may have been downloaded. The only exception I would think of as making sense is flix.jar, like it is now.

I can't immediately think of any other cases where allowing other files to survive would be anything other than confusing and error prone. Do you have anything specific in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is: lets try to be careful about what we delete.

When a path goes haywire its better to only be deleting e.g. Flix files.

Even better could we track the files we are supposed to delete?

Copy link
Member

@magnus-madsen magnus-madsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Will do an indepth review later.

Do you feel it works well?

@sockmaster27
Copy link
Contributor Author

I believe so, but the whole thing tends to be fragile in general. I had to try out some different approaches to get the tests to pass, as seemingly valid combinations of commands and actions caused them to either hang or not reset correctly.
It should be pretty solid now, but I can't guarantee that new problems won't appear in the future.

@sockmaster27
Copy link
Contributor Author

Even better could we track the files we are supposed to delete?

I tried it, but it becomes annoying when a test run is canceled mid-way (if it's hanging for example). Then that information is lost, and you'll need to delete the files manually, or the test starts acting weird.

@magnus-madsen
Copy link
Member

Even better could we track the files we are supposed to delete?

I tried it, but it becomes annoying when a test run is canceled mid-way (if it's hanging for example). Then that information is lost, and you'll need to delete the files manually, or the test starts acting weird.

OK, but then let us at least limit ourselves to deleting files with the expected extension e.g. .flix and whatelse.

@sockmaster27
Copy link
Contributor Author

The test failure is because of the Circle(Int32) vs case Circle(Int32) bug. It should otherwise be safe to merge.

@magnus-madsen
Copy link
Member

The test failure is because of the Circle(Int32) vs case Circle(Int32) bug. It should otherwise be safe to merge.

Can we mark that specific test as ignore?

@magnus-madsen
Copy link
Member

Thanks!

@magnus-madsen magnus-madsen merged commit 0cea61a into flix:master May 17, 2024
7 checks passed
@sockmaster27 sockmaster27 deleted the tests branch May 17, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants