search

exercism / go-test-runner

GNU Affero General Public License v3.0
15 stars 17 forks source link

Parsing the AST for the test_code output #19

Closed ekingery closed 3 years ago

ekingery commented 3 years ago

This PR adds parsing for top-level and subtests in Go. The README defines the specification for proper subtest parsing.

A subsequent PR could make the specification more flexible. Right now it only supports a test data definition and a range statement over the test data. The runner could, hopefully without too much difficulty, be made to accept test data definition and a range statement in any position.

A subsequent PR will also get the test runner output up to spec (see #17), and add a github action to run the tests on PR activity. We may also want to address #8.

tehsphinx commented 3 years ago

In case you plan on using https://github.com/tehsphinx/astrav to parse the AST and need some help, let me know! It should already be able to return you parts of the original code for an AST node you selected.

ekingery commented 3 years ago

In case you plan on using https://github.com/tehsphinx/astrav to parse the AST and need some help, let me know! It should already be able to return you parts of the original code for an AST node you selected.

Excellent, thanks @tehsphinx! I'll keep you posted and will definitely look into Astrav to make the AST interaction easier.

ekingery commented 3 years ago

Reminder: In the next PR, we'll want to get the test runner output up to spec (see #17), and add a github action to run the tests on PR activity. We may also want to address #8.

ekingery commented 3 years ago

Generally I'd look into protecting the indexes and handling nil pointers. When working with ASTs there are quite a few things that can be nil where you'd not expect it. Had that crash the exalysis tool quite a few times in the early times. Was nothing to worry about, as it was running locally on mentors computers, but always looked bad if a solution would come along that crashed the tool.

Thank you for the review, @tehsphinx! You are absolutely correct, this code will blow up on various tests. My goal for this PR was just to get the extraction right for tests that meet the spec (I should have said that in the PR). What I'd like to do, rather than handle each nil case, is to defer() a function that recovers from the panics whenever one is encountered in the AST processing. That should allow us to return an empty string safely whenever the AST parsing blows up. I'll make sure to get this done as part of the follow-on PR to get the output up to spec and add CI tests.