test files: rename xxx_test.sh to xxx.bats, fix shebang #541

braoult · 2021-09-15T15:18:46Z

tests scripts: change extension to .bats, fix shebang.

Reviewer Resources:

This patch includes Glennj's: exercism#25 And also includes expected_results.json fixes, as requested by Glenn on Slack: 3 spaces is fine. Just fix the expected results file to add the missing space. Don’t change anything else.

This patch includes glennj's exercism#25

iHiD

This shouldn't get merged until exercism/bash-test-runner#27 is merged

We need to be careful on this. The test runner most support both existing exercises and this new format, and that support must be deployed, before this is merged.

I'm "Requesting Changes" just to block this until the test runner is merged.

iHiD · 2021-09-16T14:21:35Z

@angelikatyborska, @neenjaw, @mpizenberg Any advice on this after doing it on your tracks?

(Thank you btw @braoult)

braoult · 2021-09-16T14:37:24Z

We need to be careful on this. The test runner most support both existing exercises and this new format, and that support must be deployed, before this is merged.

exercism/bash-test-runner#27 supports both old (xxx_test.sh) and new (xxx.bats) file names. And yes, this PR was planned to be hold until bash-test-runner PR is merged.

mpizenberg · 2021-09-16T14:43:17Z

tbh I haven't fully taken the time yet to evaluate the implications of the change on the online tooling. The tooling itself in the elm track does not support the old way, only the new one, and the filepaths changes actually occurred a while ago, before starting the work on v3. It's just that I noticed the multiple files issues on the online editor for even older submissions of mine.

braoult · 2021-09-16T14:47:53Z

tbh I haven't fully taken the time yet to evaluate the implications of the change on the online tooling. The tooling itself in the elm track does not support the old way, only the new one, and the filepaths changes actually occurred a while ago, before starting the work on v3. It's just that I noticed the multiple files issues on the online editor for even older submissions of mine.

When I discussed about this change on Slack yesterday, I wondered what could be some other side effects, nobody raised the online tooling (and it did not raise an alarm in my mind too, as I never used it :-) Could this be a blocking point ? I don't know at all how all of this interacts...

iHiD · 2021-09-16T14:53:50Z

The main effect will be that students will get both the bash and the bats files in the editor. The bash ones will be readonly and they'll have to copy their code into the bats ones.

mpizenberg · 2021-09-16T14:55:46Z

Well, the online tooling is composed of the tests runner (seems you took care of that), the analyzer (only cares about the solution), the representer (which only cares about the solution, not the tests so fine here) and the online editor, which needs to display the solution (and tests for practice exercises) to the user.

So in you case, I think the only point left is to check how is handled the presentation of the tests file in the online editor for practice exercise.

braoult · 2021-09-16T15:05:21Z

So in you case, I think the only point left is to check how is handled the presentation of the tests file in the online editor for practice exercise.

Yes.

@iHiD, why the bash (student' solution) files would be read-only, and not the batsfiles (tests) ? I am not sure to understand. What we do here is:

before :

student solution: xxx.sh
test program: xxx_test.sh

after :

student solution: xxx.sh
test program: xxx.bats

glennj · 2021-09-16T15:08:15Z

The main effect will be that students will get both the bash and the bats files in the editor. The bash ones will be readonly and they'll have to copy their code into the bats ones.

I'm not following: we're renaming the test file. In the editor, which files are available to edit? My current experience is that only the ".files.solution" file is editable, and the ".files.test" file is shown in the right-hand Tests tab. The existing "bats_extra.bats" file (test helper functions) is nowhere to be seen in the editor.

What will students have to copy?

angelikatyborska · 2021-09-16T15:48:29Z

@angelikatyborska Any advice on this after doing it on your tracks?

I actually started maintaining the track after this change happened 🤷

braoult · 2021-09-16T21:24:21Z

My concern here is only : Where and when the online editor gets the list of files (solution and test) to edit ?

where: Is there some hardcoded names on UI side, or is the list taken from somewhere (likely .meta/config.json ?). If it is taken from config.json, I don't see any issue (I updated them in this PR). If not, I simply have no clue on what should be done there.
when : I hope the filenames information is part of the student's solution environment (i.e. the filenames are determined when the student starts to use UI, and never changed later). Does someone here knows how it works, or should I ask in another repo/Slack place, with more knowledge on this part ?

angelikatyborska · 2021-09-17T04:53:59Z

where: Is there some hardcoded names on UI side, or is the list taken from somewhere (likely .meta/config.json ?).

It is taken from .meta/config.json, the three types of files - "test", "solution", and "editor" (non-editable files). But added to that are the files that the student submitted for the previous solution. Via the web editor they only could have submitted the previous values of files.solution, but via the CLI anything.

when : I hope the filenames information is part of the student's solution environment (i.e. the filenames are determined when the student starts to use UI

@iHiD will know this best, but my understanding is that every solution contains the git SHA of the exercise as it were when the student started the exercise and takes files directly from git from that point in time. However, it can change. The platform shows when a change is done to an exercise and shows you the diff between your version and the new version. Then, you can choose to update your solution to the newest version and I think that changes the SHA which means the student will now get the newest versions of all files, including the new .meta/config.json.

* Reflect bats changes (exercism/bash#541) * style

iHiD · 2021-09-17T09:37:08Z

OK, a couple of things.

The files are locked to what the student has when they start the exercise, but they are prompted to update when changes are made, at which point they get multiple solution files if the solution file has been renamed. That's because they get all their submitted files, and all the files in the solution key.
I misunderstood and thought that we were renaming the stubs, not the tests. Sorry. The fact that only the tests are changing makes this less of a concern. I'm not sure what happens when a student updates to the latest version then downloads that update. I doubt it will delete their old tests file, so they'll have both. Will that be ok?

I've now merged the test-runner one so remove my lock on this.

Test runner merged

braoult · 2021-09-17T09:50:44Z

I'm not sure what happens when a student updates to the latest version then downloads that update. I doubt it will delete their old tests file, so they'll have both. Will that be ok?

Yes: If both files are there, test-runner will use by default the new one (xxx.bats). This is done in bin/run.sh :

    local test_file="${slug//-/_}.bats"
    # test scripts may be (old) xxxx_test.sh
    [[ -f "$test_file" ]] || test_file="${slug//-/_}_test.sh"

As the test code itself is unchanged, we are fine here.

exercises/shared/.docs/tests.md

glennj · 2021-09-17T13:13:58Z

The workflow is doing this:

gh api repos/exercism/bash/pulls/541/files --paginate --jq '
  .[] |
    select(.status == "added" or .status == "modified" or .status == "renamed") |
    select(.filename |
    match("\\.(sh|bash|bats)$")) |
    .filename
'

And this finds all the renamed test scripts, and the tests all run successfully.

glennj · 2021-09-17T20:33:30Z

@IsaacG I know you have opinions about this subject. Can you review please?

braoult · 2021-09-17T20:51:28Z

@IsaacG I know you have opinions about this subject. Can you review please?

Do you mean the extension itself, as seen on Slack (no extension for executable) ?

glennj · 2021-09-17T21:08:03Z

Do you mean the extension itself, as seen on Slack (no extension for executable) ?

No, removing the extension will make the pr.yml github workflow harder to make specific.

I am hoping that Isaac can give a review faster than Jeremy.

IsaacG

This looks good to me! Thank you for your work.

IsaacG · 2021-09-17T21:40:35Z

bin/validate_one_exercise

@@ -32,4 +32,4 @@ trap cleanup EXIT
 cp "${solution}" "${exercise_name}.sh"

 # Run the tests
-bats "${exercise_name}_test.sh"
+bats "${exercise_name}.bats"


Side note/no change requested. In theory, with the fixed shebang. we should be able to give the tests a +x and just run them ... but in practice calling bats with the filename is probably easier.

Exact. I thought about it, but unsure about this +x on non *ix-like systems... And as I saw solutions (.sh) were non executable, I preferred to change nothing. Someone more aware of this should do it.

Also, I don't think that exercism download respects the executable bits.

* Reflect bats changes (exercism/bash#541) * style

braoult added 2 commits September 15, 2021 16:59

test files: rename xxx_test.sh to xxx.bats, fix shebang

9e210d0

pr.yml: remove md, add bats.

bab1e06

braoult added a commit to braoult/exercism-bash-test-runner that referenced this pull request Sep 16, 2021

Reflect bats changes (exercism/bash#541)

00d8117

This patch includes glennj's exercism#25

braoult added a commit to braoult/exercism-bash-test-runner that referenced this pull request Sep 16, 2021

Reflect bats changes (exercism/bash#541)

a4f959f

iHiD previously requested changes Sep 16, 2021

View reviewed changes

iHiD pushed a commit to exercism/bash-test-runner that referenced this pull request Sep 17, 2021

Reflect bats changes (exercism/bash#541) (#27)

0df031f

* Reflect bats changes (exercism/bash#541) * style

iHiD self-requested a review September 17, 2021 09:37

glennj requested changes Sep 17, 2021

View reviewed changes

exercises/shared/.docs/tests.md Outdated Show resolved Hide resolved

Fix a help file (thanks to Glenn)

5089252

glennj approved these changes Sep 17, 2021

View reviewed changes

Also trigger running tests for renamed files

aa9487d

braoult mentioned this pull request Sep 17, 2021

Should *_test.sh be a bats file? #405

Closed

glennj linked an issue Sep 17, 2021 that may be closed by this pull request

Should *_test.sh be a bats file? #405

Closed

glennj requested a review from IsaacG September 17, 2021 20:33

IsaacG approved these changes Sep 17, 2021

View reviewed changes

glennj merged commit 79a2d5a into exercism:main Sep 17, 2021

braoult deleted the br branch September 17, 2021 23:40

glennj pushed a commit to glennj/exercism-bash-test-runner that referenced this pull request Sep 23, 2021

Reflect bats changes (exercism/bash#541) (exercism#27)

bd3467a

* Reflect bats changes (exercism/bash#541) * style

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test files: rename xxx_test.sh to xxx.bats, fix shebang #541

test files: rename xxx_test.sh to xxx.bats, fix shebang #541

braoult commented Sep 15, 2021 •

edited

iHiD left a comment

iHiD commented Sep 16, 2021 •

edited

braoult commented Sep 16, 2021

mpizenberg commented Sep 16, 2021

braoult commented Sep 16, 2021 •

edited

iHiD commented Sep 16, 2021

mpizenberg commented Sep 16, 2021 •

edited

braoult commented Sep 16, 2021 •

edited

glennj commented Sep 16, 2021

angelikatyborska commented Sep 16, 2021

braoult commented Sep 16, 2021 •

edited

angelikatyborska commented Sep 17, 2021

iHiD commented Sep 17, 2021

braoult commented Sep 17, 2021

glennj commented Sep 17, 2021 •

edited

glennj commented Sep 17, 2021

braoult commented Sep 17, 2021

glennj commented Sep 17, 2021 •

edited

IsaacG left a comment

IsaacG Sep 17, 2021

braoult Sep 17, 2021 •

edited

glennj Sep 17, 2021

test files: rename xxx_test.sh to xxx.bats, fix shebang #541

test files: rename xxx_test.sh to xxx.bats, fix shebang #541

Conversation

braoult commented Sep 15, 2021 • edited

iHiD left a comment

Choose a reason for hiding this comment

iHiD commented Sep 16, 2021 • edited

braoult commented Sep 16, 2021

mpizenberg commented Sep 16, 2021

braoult commented Sep 16, 2021 • edited

iHiD commented Sep 16, 2021

mpizenberg commented Sep 16, 2021 • edited

braoult commented Sep 16, 2021 • edited

glennj commented Sep 16, 2021

angelikatyborska commented Sep 16, 2021

braoult commented Sep 16, 2021 • edited

angelikatyborska commented Sep 17, 2021

iHiD commented Sep 17, 2021

braoult commented Sep 17, 2021

glennj commented Sep 17, 2021 • edited

glennj commented Sep 17, 2021

braoult commented Sep 17, 2021

glennj commented Sep 17, 2021 • edited

IsaacG left a comment

Choose a reason for hiding this comment

IsaacG Sep 17, 2021

Choose a reason for hiding this comment

braoult Sep 17, 2021 • edited

Choose a reason for hiding this comment

glennj Sep 17, 2021

Choose a reason for hiding this comment

braoult commented Sep 15, 2021 •

edited

iHiD commented Sep 16, 2021 •

edited

braoult commented Sep 16, 2021 •

edited

mpizenberg commented Sep 16, 2021 •

edited

braoult commented Sep 16, 2021 •

edited

braoult commented Sep 16, 2021 •

edited

glennj commented Sep 17, 2021 •

edited

glennj commented Sep 17, 2021 •

edited

braoult Sep 17, 2021 •

edited