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

Use Verbose Unit Testing for Comprehensive Testing Summary #680

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Use Verbose Unit Testing for Comprehensive Testing Summary #680

merged 1 commit into from
Jan 11, 2024

Conversation

yctai1994
Copy link
Contributor

Description:

This pull request proposes enhancing the unit testing verbosity to provide a more comprehensive testing summary, which is particularly beneficial for CLI users. Here, I modified a single file to demo the result.

Changes Made:

  • Modified exercises/practice/pangram/runtests.jl to implement verbose unit testing.
  • Nesting was introduced in unit tests to enhance readability and organization.
  • Improved output format for better comprehension of test results.

Screenshots:

Before (Previous Testing Output):

exercism-julia-test-1

After (Enhanced Verbose Testing Output):

exercism-julia-test-2

Proposed Benefit:

It enhanced the readability and comprehensibility of testing output, especially for CLI users.

Future Scope:

If this proposal is accepted, I plan to modify the other practices' unit testing further.

Use verbose unit testing to output a more comprehensive
testing summary (particularly for the CLI users).
@ErikSchierboom
Copy link
Member

This is a great improvement and will help students better parse the test output. It also seems to be compatible with the test runner. See https://forum.exercism.org/t/use-verbose-unit-testing-for-comprehensive-testing-summary/9030/3 for the corresponding discussion.

@yctai1994 CI is failing for the test runner integration: https://github.com/exercism/julia/actions/runs/7344277878/job/20336362109?pr=680 That said, https://github.com/exercism/julia-test-runner/blob/main/test/fixtures/nested/runtests.jl seems to imply that nested tests should work, so I'm not sure if the CI script should be modified instead. Could you look into this?

Note to self: merge this with [no important files changed] when it is ready to merge.

@yctai1994
Copy link
Contributor Author

@yctai1994 CI is failing for the test runner integration: https://github.com/exercism/julia/actions/runs/7344277878/job/20336362109?pr=680 That said, https://github.com/exercism/julia-test-runner/blob/main/test/fixtures/nested/runtests.jl seems to imply that nested tests should work, so I'm not sure if the CI script should be modified instead. Could you look into this?

Ok, I am gonna take a look!

@yctai1994
Copy link
Contributor Author

@ErikSchierboom

The error

"LoadError: MethodError: no method matching ReportingTestSet(::String; verbose=true)"

seems to come from here:

https://github.com/exercism/julia-test-runner/blob/909745737f454ca92870235aa702a446fd71a64d/src/testset.jl#L18

According to the comment in the codes above, it was derived from Julia v1.4.2. On the other hand, the verbose functionality was introduced in Julia v1.6, which is the current long-term support version.

Based on the official Julia installation step, I am pretty sure the commit can work in CLI, while for the web editor...

@ErikSchierboom
Copy link
Member

It looks like the Julia test runner is on version 1.8.5 though: https://github.com/exercism/julia-test-runner/blob/909745737f454ca92870235aa702a446fd71a64d/Dockerfile

@yctai1994
Copy link
Contributor Author

Ok. So, the warning is gone after forking the repo:

https://github.com/exercism/julia-test-runner

, with a very simple workaround on line 22, from

ReportingTestSet(desc) = ReportingTestSet(desc, [])

to

ReportingTestSet(desc; args...) = ReportingTestSet(desc, [])

Should I make a pull request?

@ErikSchierboom
Copy link
Member

Yes please!

@yctai1994
Copy link
Contributor Author

Done! Here is the pull request.

@ErikSchierboom ErikSchierboom merged commit 6a88ad2 into exercism:main Jan 11, 2024
11 checks passed
@ErikSchierboom
Copy link
Member

Merged! That PR did indeed fix things. Thanks a lot!

The test results on the website now look like this:

image

What I would propose is:

  • Modify the other exercises to the new testing structure
  • Once those are all merged, modify the test runner to remove the top-level category from the test name

Would you be up for that?

@yctai1994
Copy link
Contributor Author

What I would propose is:

  • Modify the other exercises to the new testing structure

Sure, I'll gradually get them done. I think it should be better to open an issue, right? That will be able to track the progress of this task easily.

  • Once those are all merged, modify the test runner to remove the top-level category from the test name

For this part, I might need to check the details of julia-test-runner first. Once I confirm that I can address it, I'll go ahead and open a pull request there.

@ErikSchierboom
Copy link
Member

Sure, I'll gradually get them done. I think it should be better to open an issue, right? That will be able to track the progress of this task easily.

Yes please! Ping me in the issue and I'll re-open it.

For this part, I might need to check the details of julia-test-runner first. Once I confirm that I can address it, I'll go ahead and open a pull request there.

Great. It's not urgent right now, but will be once we get a bunch of these PRs merged.

cmcaine added a commit that referenced this pull request Jan 30, 2024
cmcaine added a commit that referenced this pull request Jan 30, 2024
* Reduce track-level runtests.jl output

Inspired by #680 and #686

* Less logging on GHA

The info lines are useful when run interactively so that it's clear to the user that something is happening. The GHA bot won't get impatient or confused, though, so we can skip these lines.
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