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

Implement a test runner for GDScript #2

Merged
merged 3 commits into from
Nov 1, 2023
Merged

Conversation

pfertyk
Copy link
Contributor

@pfertyk pfertyk commented Apr 3, 2023

No description provided.

@pfertyk pfertyk requested a review from a team as a code owner April 3, 2023 22:05
@pfertyk pfertyk marked this pull request as draft April 3, 2023 22:05
@@ -22,7 +22,6 @@ docker build --rm -t exercism/test-runner .
docker run \
--rm \
--network none \
--read-only \
Copy link
Member

Choose a reason for hiding this comment

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

Just to make you aware. I am rather sure that you aren't allowed to do this. I think when they run in the containers are they running in read-only environments, I will ask Jeremy or Erik but I think doing something like that aint allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ... that might be a bit of a problem :( Godot needs to create some extra folders (at least in ~/.local, I think there might be more) where the configuration of each individual project is stored. Without a permission to modify the filesystem, Godot fails to start. I might be able to create those folders manually in Dockerfile, I'll try to do that later.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we don't yet enforce read only, but we might at some point in the future. The /tmp folder is writeable, as well as the output directory passed in as the third argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ... I'll see what I can do about Godot's limitations here, in case we decide to enforce read only in the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW @ErikSchierboom , does the fact that readonly might be enforced in the future also mean that test runners shouldn't create any output files, and only use console output? Godot doesn't have an easy way to restrict the amount of printed information (e.g. when assertions are involved, or the greeting message when the binary starts), so I was going to experiment with JSON files for test results. Is this a bad idea? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've added --read-only back, but instead I created 3 volumes for directories that Godot needs to work properly (.config, .local, and .cache). If you're OK with this solution, I will gladly keep it ;)

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, but that won't work, as all test runners are mounted in the same way (so without those three new files).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ... I am not sure, but Godot might work without these folders, it will just print a lot of error messages ... I will check this. As long as the "results.json" file is produced correctly, we should be good, right?

Copy link
Member

Choose a reason for hiding this comment

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

That is correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've removed extra volumes and I'm redirecting stderr to a file in /tmp. If there is time later, I might add a filtering of those errors, so that the expected ones are ignored and the remaining ones are printed.

@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 4 times, most recently from 91e8097 to bda9dde Compare August 22, 2023 20:33
@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 2 times, most recently from ceb75f3 to 2c133f4 Compare September 3, 2023 20:51
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I'll test it functionally tomorrow!

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
bin/run.sh Outdated Show resolved Hide resolved
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Tests are working fine locally!

@pfertyk
Copy link
Contributor Author

pfertyk commented Sep 5, 2023

@ErikSchierboom Fantastic, thanks for checking! Please don't merge this yet, I would like to apply your suggestions, add more tests, and do a bit of refactoring ;)

EDIT: I realized that this is still (correctly) marked as a draft, sending a clear message that it should not be merged yet :P

@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 3 times, most recently from c0c7152 to d0d78ff Compare September 20, 2023 21:56
@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 11 times, most recently from e2c57c6 to 1370ca8 Compare September 29, 2023 19:56
@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 10 times, most recently from 6a97b5a to d06a992 Compare October 17, 2023 19:49
@pfertyk pfertyk force-pushed the gdscript-v4-test-runner branch 4 times, most recently from 5e824a5 to b01f24a Compare October 24, 2023 21:59
@pfertyk pfertyk marked this pull request as ready for review November 1, 2023 10:45
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Great work!

tests/example-empty-file/example_empty_file.gd Outdated Show resolved Hide resolved
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
@pfertyk pfertyk merged commit a9cc3ba into main Nov 1, 2023
1 check passed
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.

None yet

3 participants