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

18: Upgrade runner to meet version 2 spec #104

Merged

Conversation

cdimitroulas
Copy link
Contributor

Closes #18

The solution here relies on a custom hspec formatter which deals with producing the results.json file. In order to make this work, it uses code injection to modify the exercise test code to use this custom formatter.

Outstanding items

  • At the moment, the new bin/run.sh executes this code injection by using stack runghc. We may wish to avoid this by building an executable and executing that directly.

Copy link

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jan 28, 2024
@iHiD iHiD reopened this Jan 28, 2024
@iHiD iHiD requested a review from ErikSchierboom January 28, 2024 23:57
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! I've left some comments.

@ErikSchierboom
Copy link
Member

@cdimitroulas You'll also need to fix the tests. You can run them locally by running: ./bin/run-tests-in-docker.sh

cdimitroulas and others added 2 commits January 30, 2024 11:59
Co-authored-by: Erik Schierboom <[email protected]>
- If all tests pass, hspec formatter now correctly outputs top-level
  success status
- Implemented bash LSP hints in bin/run.sh
- Added newline at end of code injection to package.yaml files
@cdimitroulas cdimitroulas force-pushed the feat/18/upgrade-to-version-2-spec branch from 11bc796 to 2416867 Compare January 30, 2024 10:30
@ErikSchierboom
Copy link
Member

I'm doing some preliminary performance testing locally, and it does look like this new version is a bit slower. Hopefully pre-compiling will help.

@ErikSchierboom ErikSchierboom marked this pull request as ready for review January 31, 2024 08:27
@ErikSchierboom ErikSchierboom requested review from a team as code owners January 31, 2024 08:27
@ErikSchierboom
Copy link
Member

Whilst testing things locally, I'm getting this error:

Error: [S-305]
Failed to generate a Cabal file using the Hpack library on file:
/mnt/exercism-iteration/package.yaml

The error encountered was:

/mnt/exercism-iteration/package.yaml: Error while parsing $.tests.test.dependencies[1] - invalid dependency "hspec      - aeson"

I submitted these files:

src/LeapYear.hs:

module LeapYear (isLeapYear) where
 
isLeapYear :: Int -> Bool
isLeapYear year = hasFactor 4 && (not (hasFactor 100) || hasFactor 400)
  where hasFactor n = year `rem` n == 0

and

package.yaml:

name: leap

dependencies:
  - base

library:
  exposed-modules: LeapYear
  source-dirs: src

tests:
  test:
    main: Tests.hs
    source-dirs: test
    dependencies:
      - leap
      - hspec

One other request: could you make it such that after running ./bin/run-tests-in-docker.sh, the original files are restored? Now I'm getting changed package.yaml, HspecFormatter.hs and Tests.hs files.

@cdimitroulas
Copy link
Contributor Author

@ErikSchierboom I've added some automatic cleanup when running bin/run-tests-in-docker.sh or even bin/run-tests.sh.

Regarding the error you encountered, I can't reproduce that. I believe that this used to occur when you ran the test-runner multiple times without cleanup in-between because the lines appended to package.yaml would only work correctly once. I think adding this newline char at the end of the code injection should have fixed the problem though.
Could you please try cleaning up the submitted input directory's package.yaml before running again?

Note that I presume this issue would not have occurred in a real run as the input directory would contain a clean copy of the files each time.

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.

edit: see comments

@ErikSchierboom ErikSchierboom merged commit 2e6a998 into exercism:main Feb 2, 2024
1 check passed
@ErikSchierboom
Copy link
Member

Let's go!

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.

Upgrade to version 2 spec
3 participants