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

fix tests by making tests a package instead of an application #1104

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

harrysarson
Copy link
Contributor

@harrysarson harrysarson commented Apr 5, 2021

Fix tests by making the "fake application" in ./tests instead a "fake package" which helps work around the fact that elm-json does not like the fact that does not like the hacks we use to test this repo.

Old commit message

the node test runner does not like the hacks we do in
./tests/run-tests.sh. However elm-test-rs does not mind, it seems easier
to switch test runner than to update the hacks for the node test runner.

As an added extra we migrate CI from travis to github actions.

@harrysarson harrysarson force-pushed the fix-tests branch 3 times, most recently from f3bef63 to 622de43 Compare April 5, 2021 09:17
@harrysarson
Copy link
Contributor Author

Should be ready for review now :)

@harrysarson
Copy link
Contributor Author

Marking as draft because we have found a better fix in #1103

@harrysarson harrysarson changed the title fix tests by using elm-test-rs fix tests by making tests a package instead of an application Apr 16, 2021
@harrysarson harrysarson force-pushed the fix-tests branch 2 times, most recently from f450b6e to 5ced156 Compare April 16, 2021 20:21
@harrysarson
Copy link
Contributor Author

CI passes now: https://travis-ci.org/github/harrysarson/core

@harrysarson harrysarson marked this pull request as ready for review April 24, 2021 12:14
VERSION_DIR="$(ls ${ELM_HOME}/0.19.1/packages/elm/core/)"
CORE_PACKAGE_DIR="${ELM_HOME}/0.19.1/packages/elm/core/$VERSION_DIR"
# Create a link to the git package
CORE_LINK="${ELM_HOME}/0.19.1/packages/elm/core/1.0.5"

Choose a reason for hiding this comment

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

You might want to add a comment here saying that setting this to 1.0.5 is fine as long as the strategy used by the dependency solver in elm-test (here elm-json solve) prioritizes installed versions. In such case, it will use the linked 1.0.5 to the git repo even when a new 1.0.6 version of core exists. This of course only holds true as long as the build-time and run-time tests dependencies do not require a version of elm/core > 1.0.5.

Otherwise, we need to bump this 1.0.5 once there is a new release of elm/core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this: a888bfe hopefully this will keep us in sync regardless of the dependency resolution

Copy link

Choose a reason for hiding this comment

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

I suppose this does the trick in 99% of the cases. The only exception I can think of is for the commit that changes the package version. It will try to use the new version that does not exist in the package server yet and thus will make the elm compiler fail I think. Might be annoying if this shows a red failure for CI on the commit corresponding to new package push.
What do you think?

PS: except if your elm publish is faster than your CI but that's a bit playing the daredevil ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, this is a problem. Maybe we need to go back to the old approach of running elm-test once to prepopulate elm_home and then simlink

Choose a reason for hiding this comment

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

Well you could also just ask the package server which version of elm-core is the latest.

Copy link

Choose a reason for hiding this comment

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

edited

curl -L https://package.elm-lang.org/packages/elm/core/releases.json | jq -r 'keys | .[-1]'
# 1.0.5

@rupertlssmith
Copy link

I would like to add this PR to https://github.com/elm-janitor/core.

I think this would be better squashed to a single commit with git rebase -i, would it be possible to do that?

@harrysarson
Copy link
Contributor Author

I have given you commit rights on my fork, please feel free to squash these commits on the harrysarson:fix-tests branch.

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.

None yet

3 participants