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

Put exercise generation under test #426

Merged
merged 2 commits into from
Oct 20, 2020
Merged

Put exercise generation under test #426

merged 2 commits into from
Oct 20, 2020

Conversation

samWson
Copy link
Contributor

@samWson samWson commented Oct 16, 2020

This change puts the exercise generation for V2 under test. With this in place we can start work on a new generator for V3. My approach will be to create a duplicate of the V2 generator and alter it for V3 output. We can then start moving common behavior into a superclass. With the tests in place this becomes much easier to do while maintaining existing functionality.

Commit Message

This change alters ExercismGenerator just enough to allow side effect free
tests. Having the class under test enables future refactoring. Three dependencies
made this class difficult to test:

  • PipeableOSProcess executed commands outside of the Pharo VM
  • ExTonelWriter wrote files to the OS filesystem
  • ExercismExercise would access a variable number of exercises depending on how many were implemented

To get this class under test the above dependencies have been placed in lazy
assigned instance variables. This allows tests to assign mocks to the generator
instance.

There are two mock OS processes, one that always succeeds, and one that always fails.
Neither executes any function outside the VM.

A ExTonelWriter writes exercises to an in memory file system that lasts only
for the test.

A mock exercism exercise class returns only a single exercise. This speeds up the
test and makes it more consistent as it won't change when more exercises are implemented.

This change alters `ExercismGenerator` just enough to allow side effect free
tests. Having the class under test enables future refactoring. Three dependancies
made this class difficult to test:
- `PipeableOSProcess` executed commands outside of the Pharo VM
- `ExTonelWriter` wrote files to the OS filesystem
- `ExercismExercise` would access a variable number of exercises depending on how many were implemented

To get this class under test the above dependancies have been placed in lazy
assigned instance variables. This allows tests to assign mocks to the generator
instance.

There are two mock OS processes, one that always succeeds, and one that always fails.
Neither executes any function outside the VM.

A  `ExTonelWriter` writes exercises to an in memory file system that lasts only
for the test.

A mock exercism exercise class returns only a single exercise. This speeds up the
test and makes it more consistent as it won't change when more exercises are implemented.
@samWson
Copy link
Contributor Author

samWson commented Oct 16, 2020

One of my tests broke in CI. I'll take a look.

The change is a test that passed on my personal computer (MacOS) but failed in CI (Ubuntu 16.04).

I suspect that even though the tests use an in memory file system OS specific line-feed and carrage-return
characters will be used causing enough of a difference between the expected result and the actual
result of the test to fail. This change makes the line ending characters of the actual and expected results
identical before asserting.

The test design closely follows that of `TonelWriterTest` which also uses an in memory file system.
@samWson
Copy link
Contributor Author

samWson commented Oct 17, 2020

The failing test might be due to a difference in line ending characters for the OS running the test and the line endings in the expected test results. I've forced the expected results and the actual results to use the same line endings before assertions are made.

This seems to have worked. CI is now green.

@samWson
Copy link
Contributor Author

samWson commented Oct 18, 2020

@exercism/reviewers could I please have someone review this change? It's in Smalltalk so if you know a bit of Ruby you will at least know the OOP side of things. I'm happy to talk you through everything else.

Copy link
Member

@SleeplessByte SleeplessByte left a comment

Choose a reason for hiding this comment

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

It looks fine to me :3

I think you're right on the "line ending" issue when files are compared. Even accounting for OS line endings isn't good enough because .gitattributes and git local and global config can override it. Normalizing line-endings is usually the way to go.


We have to do it this way as Exercism conventions differ from Tonel, and so we need to output them to a seperate directory suitable for the Exercism command line tool.
I am responsible for generating kebab-cased Exercism V2 directories, each containing a seperate exercise for users.
Copy link
Member

Choose a reason for hiding this comment

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

(is it common in smalltalk that classes are personified?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you make a class for the first time the default class comment template has an example that includes For example, "I represent a paragraph of text". I took a quick look through some of the class library. It seems to be a common, but not universal practice. I've stuck with the convention ever since I saw it although I don't use it in any other programming language.

@macta
Copy link
Contributor

macta commented Oct 19, 2020 via email

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.

I honestly can't comment on the code (except for that it looks really well documented!), so I'll approve :)

@samWson
Copy link
Contributor Author

samWson commented Oct 20, 2020

Thanks for the interesting bit of info @macta. @SleeplessByte @ErikSchierboom thanks for the reviews. Merging now.

@samWson samWson merged commit 2ac04ea into exercism:master Oct 20, 2020
@samWson samWson deleted the generator-refactor branch October 20, 2020 08:55
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.

4 participants