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

Add a testing script #6

Merged
merged 4 commits into from
Sep 19, 2021
Merged

Add a testing script #6

merged 4 commits into from
Sep 19, 2021

Conversation

glennj
Copy link
Contributor

@glennj glennj commented Aug 30, 2021

Requires Process.chdir(_) from wren-console and Strings.globMatch(_, _) from wren-essentials
Latest iteration no longer has these requirements.

Requires `Process.chdir(_)` from wren-essentials
@glennj glennj changed the title WIP: run all tests Add a testing script Sep 17, 2021
var testDir = "./tests/"
var testFiles = Directory.list(testDir)
.where {|f| Strings.globMatch(f, "*.wren")}
.map {|f| testDir + f[0..-6]}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this just stripping extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Owner

Choose a reason for hiding this comment

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

We need a nicer way, but we have no basename yet. :)

Comment on lines 13 to 16
var tmpfile = ".git.root.%(Process.pid)"
Process.exec("sh", ["-c", "git rev-parse --show-toplevel > %(tmpfile)"])
var rootdir = File.read(tmpfile).trim()
File.delete(tmpfile)
Copy link
Owner

Choose a reason for hiding this comment

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

Is all this because you don't often work out of the root directory of projects? Many of my utility scripts ASSUME this... this isn't bad, but it's also not entirely portable...

Can we get the absolute pathing given argument 0, the script name itself? Or is that stripped of path info?

Copy link
Owner

Choose a reason for hiding this comment

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

It's not exactly public but you have the Path class at your disposal as well for navigation around a path string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ cd tests
$ cat > path.wren
import "os" for Process
System.print(Process.arguments)
System.print(Process.allArguments)
System.print(Process.cwd)
^D
$ wrenc ./path.wren
[]
[wrenc, ./path.wren]
/Users/glennjackman_1/src/other/wren/joshgoebel/wren-testie/tests

It's just because the error message for running a test file is so maddening unless you're at the repo root

$ wrenc ./expect.wren
Could not load module 'testie'.
at (script) (././expect.wren line 1)

I haven't really gotten the module resolver. I thought imports of relative paths are relative to the directory of the file currently being evaluated, and this is why scripts/test_all.wren has to import "../testie" -- but then that error message baffles me: relative to tests/expect.wren, testie is in ../

Copy link
Owner

Choose a reason for hiding this comment

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

I haven't really gotten the module resolver. I thought imports of relative paths are relative to the directory of the file currently being evaluated,

Correct. And the original script (first argument to wrenc) root is used for walking up the directory to find wren_modules folders.

Pretty sure there is a bug in Path class here:

  • ./../testie normalizes to testie, which is wrong.

PR forthcoming.

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Don't assume we're bug free. :) Might spend lots of time writing code to get around bugs that are much simpler to fix than the code you're writing to avoid them. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking I should remove the check for git rootdir, and maybe just replace it with a comment.

Copy link
Owner

Choose a reason for hiding this comment

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

You try my PR and see if it works fine without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work fine, but because I have to list a relative dir, I'm pretty much stuck at forcing execution from the project root.

Copy link
Owner

Choose a reason for hiding this comment

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

Huh?

@joshgoebel
Copy link
Owner

joshgoebel commented Sep 17, 2021

Ok, now we have an issue where this wouldn't just work with the release version of wrenc... is it's the script job to check the runtime version, or should the package manager handle that? I worry someone doing development of the library isn't using the package manager though so it would never flag since it was not being used for dev work...

I'm wondering if we should have some sort of Runtime.assertVersion in wren-console itself perhaps - since we're still so young and changing?

@glennj
Copy link
Contributor Author

glennj commented Sep 17, 2021

I'm wondering if we should have some sort of Runtime.assertVersion in wren-console itself perhaps - since we're still so young and changing?

I'd say it's the script's job since wren is not very introspective, and that assertVersion is a good idea.

@joshgoebel
Copy link
Owner

scripts/test_all.wren Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Owner

Oh that might be nice. Excited to try this later.

@joshgoebel joshgoebel merged commit 564d2f6 into joshgoebel:main Sep 19, 2021
@glennj glennj deleted the testing-script branch September 19, 2021 13:57
glennj added a commit to glennj/wren-testie that referenced this pull request Sep 19, 2021
* Add a testing script.
Requires `Process.chdir(_)` from wren-essentials

* do not show empty lists

Co-authored-by: Josh Goebel <me@joshgoebel.com>
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