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

Hoist CI build requirements to top of issue template #12

Closed
wants to merge 2 commits into from

Conversation

jtigger
Copy link

@jtigger jtigger commented Mar 23, 2017

Feels like a lot of unilateral decisions in this PR; I hope I'm understanding the purpose and audience.

  1. suggesting that we make explicit all the "requirements" of setting up CI at the very top of the file (the what).
  2. elaborate the background on each "requirement" with similarly named sections (the why).
  3. keep the excellent examples at the tail of the file (the how).

Also, there is background for writing high quality exercise test suites. Suggesting we extract these to a separate file. In fact, I believe this would go in the new docs repo in some section giving background on writing test suites.

- extract background for writing high quality exercise test suites to a separate file.
@kytrinyx
Copy link
Member

In fact, I believe this would go in the new docs repo in some section giving background on writing test suites.

Yes, I think that is right. I think that probably goes in contributing-to-language-tracks, but maintaining-a-track would also be correct.

Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

This is great. I love the direction.

TRAVIS Outdated

- for each exercise in the track,
- run that exercise's test suite (aka the "exercise test suite");
- use the exercise's solution as the code under test.
Copy link
Member

Choose a reason for hiding this comment

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

"sample solution" or "example solution"?

Copy link
Author

Choose a reason for hiding this comment

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

👍 the tooling is looking for an "example" solution, that one.

TRAVIS Outdated
- for each exercise in the track,
- run that exercise's test suite (aka the "exercise test suite");
- use the exercise's solution as the code under test.
- ensure all test cases are enabled;
Copy link
Member

Choose a reason for hiding this comment

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

Should ensuring all test cases are enabled be first in the list?

Copy link
Author

Choose a reason for hiding this comment

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

Noice. In fact, I think it would best serve as a sub-bullet of "run that exercise's test suite".

TRAVIS Outdated
- use the exercise's solution as the code under test.
- ensure all test cases are enabled;
- be executed on Travis CI;
- be easy to run on a contributor/maintainer's local machine (i.e. a build script with any needed instructions).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. Instructions. Let's specify that this needs to be documented in the README of the track.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a placeholder for this as the last bullet under "Submitting a Pull Request" in the TRACK_README template.

Copy link
Author

Choose a reason for hiding this comment

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

@parkerl would that bullet contain the instructions to run the script? Or would it reference those instructions? (on some tracks, there might be a little more setup surrounding the script execution... ...cough... Java ...cough... for example ...wheeez...).

TRAVIS Outdated

Many test frameworks will randomize the order of the tests when running them. This is an excellent practice, which helps ensure that subsequent tests are not dependent on side effects from earlier tests. However, in order to simulate TDD we want tests to run *in the order that they are defined*, and we want them to *fail fast*, that is to say, as soon as the test suite encounters a failure, we want the execution to stop. This ensures that the person implementing the solution sees only one error or failure message at a time, unless they make a change which causes prior tests to fail.
As described in [Contributing to Exercism: Writing exercise test suites](writing_tests.md), exercise test suites are written such that all but the first test case are skipped/pending/ignored. For the purpose of the track test suite, all test cases should be enabled (simulating an Exercism user having completed the exercise).
Copy link
Member

Choose a reason for hiding this comment

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

  • s/Writing/writing/
  • s/writing_tests.md/writing-tests.md

exercise test suites are written such that all but the first test case are skipped/pending/ignored.

s/test suites/test suites in most language tracks/. Some language tracks are able to implement this without skipping, pending, ignoring (e.g. Go has a t.Fatal() that will stop on the first error or failure, whereas t.Error() will report the error and move on. I think also some language tracks have decided not to fail fast. That might be a decision that we need to consider when we start looking at the curriculum from a high level and consider the user experience.

I really like the simulating an Exercism user having completed the exercise bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

TRAVIS Outdated

Whatever the mechanism—functions, methods, annotations, directives, commenting out tests, or some other approach—these are changes made directly to the test file. The person solving the exercise will need to edit the test file in order to "activate" each subsequent test.
It's important that if we rewrite files in any way during a test run, that these changes do not accidentally get checked in to the git repository.
Copy link
Member

Choose a reason for hiding this comment

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

s/git/Git/ (Git is a proper name)

TRAVIS Outdated

Any tests that are marked as skipped will not be verified by the track test suite unless special care is taken.
Therefore, many language tracks write the track test suite in such a way that it _copies_ the exercise to a temporary location outside of the git repository before editing or rewriting the exercise files during a test run.
Copy link
Member

Choose a reason for hiding this comment

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

s/git/Git

TRAVIS Outdated

Additionally, in some programming languages, the name of the file containing the solution is hard-coded in the test suite, and the example solution is not named in the way that we expect people to name their files.
_Configuring Travis CI_
Copy link
Member

Choose a reason for hiding this comment

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

I now realize that we sometimes have AppVeyor (Microsoft stack) and Circle CI (Apple stack). Travis CI only runs on Linux.

TRAVIS Outdated

**Avoiding accidental git check-ins**
Copy link
Member

Choose a reason for hiding this comment

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

And I clearly forgot that Git is a proper name. Shame on me.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

@@ -0,0 +1,16 @@
_(in fact, this would be part of the new "docs" repo, not a file in this repo.)_
Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Part of this was stolen by "someone" and placed in the TRACK_README. I agree a part of it should also go into docs but some of this about randomization is relevant here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, how about a one-liner that links to the docs?

Copy link
Author

Choose a reason for hiding this comment

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

... Also, I move to hunt down that "someone" and bring him to justice! ;) 🍻

Copy link
Author

Choose a reason for hiding this comment

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

(clears his throat to appear serious) So, which parts do you feel should be in-lined, @parkerl ? As I'm suggesting in this PR, we're really nudging pretty hard to go read that section in the central guide. I'm of the mind that these characteristics (including how some tooling randomizes tests while others don't) are really part and parcel to this general discussion about test suites.

Copy link
Contributor

@parkerl parkerl left a comment

Choose a reason for hiding this comment

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

Nice work @jtigger

TRAVIS Outdated
- use the exercise's solution as the code under test.
- ensure all test cases are enabled;
- be executed on Travis CI;
- be easy to run on a contributor/maintainer's local machine (i.e. a build script with any needed instructions).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a placeholder for this as the last bullet under "Submitting a Pull Request" in the TRACK_README template.

TRAVIS Outdated

Many test frameworks will randomize the order of the tests when running them. This is an excellent practice, which helps ensure that subsequent tests are not dependent on side effects from earlier tests. However, in order to simulate TDD we want tests to run *in the order that they are defined*, and we want them to *fail fast*, that is to say, as soon as the test suite encounters a failure, we want the execution to stop. This ensures that the person implementing the solution sees only one error or failure message at a time, unless they make a change which causes prior tests to fail.
As described in [Contributing to Exercism: Writing exercise test suites](writing_tests.md), exercise test suites are written such that all but the first test case are skipped/pending/ignored. For the purpose of the track test suite, all test cases should be enabled (simulating an Exercism user having completed the exercise).
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

TRAVIS Outdated

**Avoiding accidental git check-ins**
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

@@ -0,0 +1,16 @@
_(in fact, this would be part of the new "docs" repo, not a file in this repo.)_
Copy link
Contributor

Choose a reason for hiding this comment

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

Part of this was stolen by "someone" and placed in the TRACK_README. I agree a part of it should also go into docs but some of this about randomization is relevant here.

@jtigger
Copy link
Author

jtigger commented Mar 28, 2017

Okay, I'm really diggin' the copy editing! Thank you for the thorough review, folks.

I believe I addressed the points brought up. A little discussion still about landing spots for a few bits.

@kytrinyx
Copy link
Member

A little discussion still about landing spots for a few bits.

Would you rephrase? I can't figure out how to parse this. (Sorry! I am traveling, brain might not actually be working right) 🌷

@jtigger
Copy link
Author

jtigger commented Apr 4, 2017

Sorry for the delay, @kytrinyx. In fact, I missed your suggestion to put in a one-liner and link to the docs. That's a perfect solution, here.

That reference would be to the page requested by exercism/docs#8. I'm gonna update that issue to request the reference be made as part of that work.

@parkerl
Copy link
Contributor

parkerl commented May 2, 2017

@jtigger I'm lost. Where are we with this one?

@kytrinyx
Copy link
Member

I ended up removing the issue template, as it contained too many assumptions about the state of the track. I'm going to go ahead and close this issue.

@kytrinyx kytrinyx closed this Feb 26, 2019
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