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

Add fixtures to aruba #224

Merged
merged 3 commits into from Apr 29, 2015
Merged

Add fixtures to aruba #224

merged 3 commits into from Apr 29, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jan 15, 2015

This PR adds the basics to support fixtures in aruba. Also see #219.

Things to discuss:

  • Use of OpenStruct

    I used OpenStruct here to make both paths of a fixture available: The relative one which is used to find a fixture and the absolute one, which can be used by a consumer of the api like copy_file. I can replace this by a Hash if this is preferred. Though OpenStruct is part of ruby core distribution.

  • Use of Pathname

    I like the relative_path_from-functionality of Pathname. It makes the intention of the author clear and is quite easy to use to build relative paths.

  • Use of %/ as fixture path prefix

    It does not have a special meaning in ruby so far - besides percent classes which are not followed by /.

  • Order of directories to be searched

    I use "my" preferred order here, but I'm open to change it.

  • Commit Style

    I will refactor the "commits" after all things above are discussed to make sure that there's one commit for the tests and one commit for the implementation.

  • Rubocop

    I think we will need to "guard" this line and that line here because of === (3x =). But === returns true'/falsefor bothStringANDRegexpand thus is more flexible than==`

@ghost
Copy link
Author

ghost commented Jan 23, 2015

Any comments about this one @jarl-dk ?

@ghost
Copy link
Author

ghost commented Feb 25, 2015

@jarl-dk ping... 😄

@maxmeyer
Copy link
Member

This PR has been opened for a while.

@aslakhellesoy @mattwynne @jarl-dk Is there anyone willing to review this? I'm a bit frustrated waiting so long for feedback.

@mattwynne
Copy link
Member

I don't understand what a fixture is in this context. Is there an example somewhere of how this feature would be used in practice? Maybe add something to the README?

@ghost
Copy link
Author

ghost commented Mar 17, 2015

This PR is a result of the discussion in #219.

I added an example to the readme. Though I must admit, that this PR only adds some more basic stuff which will become really useful when #219 (Add copy method) is finished and makes use of this. Until then the example in the readme feels a little bit unfinished.

@jarl-dk
Copy link
Member

jarl-dk commented Mar 26, 2015

I'll take a look soon.

@jarl-dk jarl-dk self-assigned this Mar 26, 2015
@jarl-dk
Copy link
Member

jarl-dk commented Apr 4, 2015

@mattwynne : In this context a fixture is a constant byte sequence (stored as a file). Such fixture files are sometimes used to compare binary data against. It could be images or other binary static data that you verify against in a test. e.g.
check_binary_file_content('produced_file.jpeg','fixtures/expected_file_content.jpeg')
such fixtures are often stored in spec/fixtures, features/fixtures, or test/fixtures.
Does it make sense?

@mattwynne
Copy link
Member

mattwynne commented Apr 5, 2015 via email

@jarl-dk
Copy link
Member

jarl-dk commented Apr 6, 2015

@mattwynne : As you can imagine the example
check_binary_file_content('produced_file.jpeg','fixtures/expected_file_content.jpeg') will not really work because fixtures are stored in spec/fixtures, features/fixtures, or test/fixtures and the above file paths (especially) the second parameter are relative to current working directory which is a new directory in every run (under tmp/aruba. So the purpose of this PR is to introduce a special character (just like ~ for HOME) that expands to the directory with fixtures. This specific PR proposes to use % for that turning the example into check_binary_file_content('produced_file.jpeg','%/expected_file_content.jpeg'), but other characters have been proposed (such as -, |, ^, >) starting at #219 (comment)
Personally I prefer a single character and |, <, and > may be confused with shell piping.

@jarl-dk
Copy link
Member

jarl-dk commented Apr 6, 2015

Actually I have often used fixtures (images) to test web-applications using cucumber. So in some sense I see the fixture expansion feature (the purpose of this PR) to be more generally useful than just for CLI testing. So someday I see this feature being implemented in a more general ruby gem (cucumber or even better: mini-test). But maybe we should just start here in Aruba to demonstrate proof-of-concept.

DEFAULT_ROOT_DIRECTORY = Dir.getwd

# The root directory of aruba
def root_directory
Copy link
Member

Choose a reason for hiding this comment

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

Making aruba root directory configurable is not related to this PR (as I see it). I suggest you take that out in a seperate PR.

Copy link
Author

Choose a reason for hiding this comment

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

I see, but it makes the configuration of the fixtures more cleaner. I added a new PR for this. Would you mind to merge it first, then?

@ghost ghost mentioned this pull request Apr 7, 2015
@ghost
Copy link
Author

ghost commented Apr 7, 2015

I cleaned up the implementation and rebased it on master... BTW I left the root_directory-implementation within this PR, but opened a new PR #232 for adding it. I will rebase this PR when/if #232 is merged.

@jarl-dk
Copy link
Member

jarl-dk commented Apr 7, 2015

I was a bit fast on criticising absolute_path. I see that it was already part of v0.6.2.

I am glad that you take the time to rename it and make absolute_path deprecated.

def absolute_path(*args)
in_current_dir { File.expand_path File.join(*args) }
warn('The use of "chmod" is deprecated. Use "filesystem_permissions" instead')
Copy link
Member

Choose a reason for hiding this comment

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

Looks like copy paste code from chmod

Copy link
Author

Choose a reason for hiding this comment

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

ups again. Will be fixed...

@ghost
Copy link
Author

ghost commented Apr 7, 2015

@jarl-dk Fixed.

@ghost
Copy link
Author

ghost commented Apr 8, 2015

rebased with master. waiting for #232 to be merged and then I will rebase again.

@ghost
Copy link
Author

ghost commented Apr 27, 2015

@jarl-dk @mattwynne Anything missing here? Would love to see this merged soon. Please be aware that this PR includes code from #232. So it makes sense to merge #232 first.

@ghost ghost mentioned this pull request Apr 28, 2015
@jarl-dk jarl-dk merged commit 5dd8393 into cucumber:master Apr 29, 2015
jarl-dk added a commit that referenced this pull request Apr 29, 2015
@jarl-dk
Copy link
Member

jarl-dk commented Apr 29, 2015

I have rework abit, @dg-ratiodata Please review my rework. Still I think this feature is not exploited in feature tests where I think it belongs most. Basically the new Aruba#expand_path should be used everywhere in lib/aruba/cucumber.rb so the fixtures file path can be used in feature steps. So some more work is needed there...

@ghost ghost mentioned this pull request Apr 30, 2015
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