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

travis: Support multiple example directories; anagram: Add multiple examples #396

Merged
merged 3 commits into from
Oct 12, 2016
Merged

travis: Support multiple example directories; anagram: Add multiple examples #396

merged 3 commits into from
Oct 12, 2016

Conversation

petertseng
Copy link
Member

Closes #395 (though I will plan to open a new issue to be able to mark an example as should-fail. Perhaps with a SHOULDFAIL in its directory name?)

Individual commits should explain well enough.

mv ${example}/package.yaml .
found=1

# ARGH, DUPLICATE CODE
Copy link
Member Author

@petertseng petertseng Oct 12, 2016

Choose a reason for hiding this comment

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

please help if you can help avoid this duplicate code. use a function?

test_exercise () {
    stack test ${SET_RESOLVER} ...
}

and then just call test_exercise ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't worry with that for now. Let's just add the features and later we can simplify everything. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

The function worked fine so I am going to use it.

--pedantic `# Enable -Wall and -Werror. `
done

if [ -z "$found" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

kind of a clumsy way to do it, but I think it'll work. Any better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not yet...but it is good enough the way it is. 😄

@@ -0,0 +1,17 @@
name: anagram
Copy link
Member Author

Choose a reason for hiding this comment

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

this file is pretty much just "the default package.yaml" - perhaps we should support the absence of this file, in which case the one from exercises/anagram/package.yaml is used? the only tricky bit is that it may be overwritten by an earlier example, so we need to back it up beforehand.

Copy link
Member Author

Choose a reason for hiding this comment

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

The advantage would be... fewer package.yaml to have to keep up to date if we have to change them all?

the disdavntage would be that the test code would get slightly more complex.

Copy link
Contributor

@rbasso rbasso Oct 12, 2016

Choose a reason for hiding this comment

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

I would not bother with support for a distinct package.yaml for now. We can solve that later.

test_exercise
done

if [ -z "$found" ]; then
Copy link
Member Author

@petertseng petertseng Oct 12, 2016

Choose a reason for hiding this comment

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

I think this does not work as intended. On my computer, at least, if examples dir is empty or does not exist, this is wha thappens:

testing examples/*
mv: cannot stat 'examples/*/*.hs': No such file or directory
mv: cannot stat 'examples/*/package.yaml': No such file or directory

As you can see, since the glob found nothing, it is interpreted literally and we proceed to a single iteration of the for loop. And then mv fails, which I assume will abort the script as it should. But that means this found is unnecessary.

So, is ther ea point in keeping it?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me that tells me there is no point in keeping it. I'll remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I can keep it, I suppose, in case there are shells with different semantics for empty globs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found one link about it, but I couldn't test test it yet.
http://stackoverflow.com/questions/2937407/test-whether-a-glob-has-any-matches-in-bash

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, I'll do one with stat

Copy link
Contributor

@rbasso rbasso Oct 12, 2016

Choose a reason for hiding this comment

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

Do you want to change anything else or can I merge now?

Nice work, BTW. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I am satisfied!

This serves a few purposes:

* Validating that a test suite can accept multiple possible type
  signatures, such as anagram in #393.
* Isolating the `package.yaml` for each example, preventing us from
  having to show students our example's `package.yaml`.
* (With a future extension that marks an example as should-fail) allows
  us to make sure our test suite catches certain solutions and causes
  them to fail.

Closes #395
This is the first step as a proof of concept that the examples directory
will work and will be tested by Travis CI.

The next step would be to add other examples alongside this one.

As part of the work in #395.
This is a second example that will fully test (using the proposal #395)
that the new tests in #393 correctly accept multiple signatures.

This is simply the old example before #395 changed it from acting on
`[String]` to `Set Text`
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