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

Issue #154: make journey-test.sh use exercises #277

Merged

Conversation

FridaTveit
Copy link
Contributor

Made bin/journey-test.sh iterate over exercises instead of problems. I've tested the individual lines I've changed and they give the same output as when they were iterating over problems. However I've not been able to run the entire script to test it... Any suggestions for how to do that are very welcome! :)

@jtigger
Copy link
Contributor

jtigger commented Feb 3, 2017

Yup and this script runs in CI and that works perfectly. I'm gonna "review" this as approved; feel free to merge whenever you're ready, @FridaTveit.

It is a design goal of the script to both fail fast and give good signal as to what's wrong when something goes wrong. I'd be happy to work through whatever you're seeing to get it running locally.

What happens when you run the script?

@FridaTveit
Copy link
Contributor Author

Thanks! :) Well it had lots of complaints about ruby (first that I didn't have it, then that it didn't like version 2.2.6, then that gem needed some update) that I've managed to fix. But now it's saying bin/journey-test.sh: line 180: ./exercism: cannot execute binary file: Exec form at error

@jtigger
Copy link
Contributor

jtigger commented Feb 4, 2017

Looks like the attempt to detect which OS your machine has failed.

What do you get when you run:

$ uname

(p.s. I'm online right now; find me at https://gitter.im/exercism/xjava or https://appear.in/exercism/xjava)

@stkent
Copy link
Contributor

stkent commented Feb 4, 2017

I'm not a rubyist, but this line would appear to reference a Gemfile that's not part of the repo?

https://github.com/exercism/xjava/blob/68684542b474ed154908f9b579f8ec7505a2f083/bin/journey-test.sh#L33

@FridaTveit
Copy link
Contributor Author

From what I remember from trying to debug this, that Gemfile is from x-api which the script fetches at some point?

An update on how I got on with this: I managed to get most of the script working with help from @jtigger on Saturday. Thanks! :) But now I'm getting this error:

+ ./exercism --config .journey-test.exercism.json fetch java $'hello-world\r'
::1 - - [04/Feb/2017:16:24:28 +0000] "GET /v2/exercises/java/hello-world%0D HTTP/1.1" 404 64 0.0200
2017/02/04 16:24:28 unable to fetch problems (HTTP: 404) - No implementation of ' in track 'java'

@jtigger
Copy link
Contributor

jtigger commented Feb 8, 2017

Hey Frida... I notice that in the "GET" there's an extra character...

GET /v2/exercises/java/hello-world%0D

That 0x0D or decimal 13: the carriage return character......

oh! I see it right there in the command-line:

./exercism --config .journey-test.exercism.json fetch java $'hello-world\r'

🤷‍♂️ ... looking into it...

@FridaTveit
Copy link
Contributor Author

Yes, I did notice that but I have no idea where the \r comes from... By the way, should I add the changes I made to the script to make it run on my machine to this review?

@FridaTveit
Copy link
Contributor Author

I just tried running the script with it iterating over problems instead of exercises and I get the same error. So I guess it's just something strange to do with runnning it on my machine. The script seems to really not like my laptop :P

@jtigger
Copy link
Contributor

jtigger commented Feb 8, 2017

Hey now! Well then this script needs a few more lessons in diplomacy 'cause it needs to be running on whatever machine.

I suspect we can tweak this jq query to not emit the CRs... My work day is firing off, so I can't dig in further right now.

@jtigger
Copy link
Contributor

jtigger commented Feb 9, 2017

I was able to get the exercise slugs on a single line like this:

cat config.json | jq '.exercises[].slug + " "' --join-output

Does that help?

@FridaTveit
Copy link
Contributor Author

Great, that worked thanks! :) Now it's just complaining about the symlinks and from issue #148 it looks like they won't work on Windows. So maybe I should just admit defeat as it looks like this is as far as it'll get on my machine... :P

@jtigger
Copy link
Contributor

jtigger commented Feb 10, 2017

I'd rather dereference the symlinks (i.e. remove them and copy the files) and rely on the journey test to catch problems, than have a core maintainer not have the ability to run the suite on her or his machine (of the OS of her or his choosing).

I'm not saying you have to do that work, Frida, just that I'd rather it go that way.

@FridaTveit
Copy link
Contributor Author

Sure, that's what I've been doing locally (removing them and copying the files) when I've needed to run tests with symlinks. I just didn't want to cause lots of problems with my uncooperative OS :P

But I'm happy to do that work :) Or do you think they should be good first patch issues?

@jtigger
Copy link
Contributor

jtigger commented Feb 10, 2017

But I'm happy to do that work :) Or do you think they should be good first patch issues?

I see this as "discovered scope" in this PR — and should be done as part of getting the script "up-to-date". Yes, please!

To put a finer point on all this: maintainers are an Free Open Source Software product's lifeblood. Without maintainers, very little gets done. It's a happystance that we, as maintainers, are delighted by the sheer doing of the work. But in every way that is in our control, the happy part of that stance should be cultivated. If there's an apparent conflict between some other need and what would make a maintainer's life better, the latter should usually prevail.

This may seem like a little thing: copying files where there are symlinks. But even the little things add up.

Thanks for volunteering to do this, @FridaTveit!

@stkent stkent mentioned this pull request Feb 11, 2017
@FridaTveit
Copy link
Contributor Author

Yay, it finally runs on my machine! Happy days! :D

@stkent
Copy link
Contributor

stkent commented Feb 12, 2017

Awesome, nice work @FridaTveit!

@FridaTveit FridaTveit merged commit 6e624c0 into exercism:master Feb 13, 2017
@FridaTveit FridaTveit deleted the JourneyTestMakeWorkWithExercises branch February 13, 2017 09:56
@jtigger
Copy link
Contributor

jtigger commented Feb 13, 2017

Nice work, indeed, @FridaTveit! I'm also starting to help maintain the Kotlin track. "Stealing" your work to update the journey test over there too. So, your efforts are immediately doubled!!

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.

3 participants