Sitelet https://github.com/elastic/docs/pull/810
Skip to content

Integtest: Add tests for building all books - #810

Merged
nik9000 merged 5 commits into
elastic:masterfrom
nik9000:rspec_integtest_all
Apr 19, 2019
Merged

nik9000 merged 5 commits into
elastic:masterfrom
nik9000:rspec_integtest_all

Conversation

@nik9000

@nik9000 nik9000 commented Apr 11, 2019

Copy link
Copy Markdown
Contributor

Adds a basic test for building all books. I expect we'll want to refactor
it significantly as we port more tests for Make to rspec, but this is a
start!

Adds a basic test for building all books. I expect we'll want to refactor
it significantly as we port more tests for Make to rspec, but this is a
start!
@nik9000
nik9000 requested a review from estolfo April 11, 2019 21:39
Some text.
ASCIIDOC
src.init_repo 'source'
src.write 'conf.yaml', <<~YAML

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I'll end up having to make generating this file more parameterized and automatic but I'd like to wait for a follow up for that.


##
# Init a git repo in root and commit any files in it.
def init_repo(root)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've moved this to the Source object because we'll want to initialize many repos when testing --all

dest_file(file_name)
end
let(:contents) do
return unless File.exist? file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks strange to me to have a return in a let block. Maybe you can just write the line in a if File.exist? file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does feel weird? I'm not really sure the if File.exist? block is super clear either. I think for me coming from Java the return unless is more clear but I can't speak to idiomatic ruby.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess I think of let blocks not quite as methods, more like procs. But this is not a critical thing and if you prefer using return, that's no problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Let me see if I can resolve the merge conflicts here.....

@nik9000 nik9000 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@estolfo, could you have another look at this one when you get a chance? Merging master required I do a little reorganization!

@nik9000
nik9000 merged commit 15bf3c0 into elastic:master Apr 19, 2019
@nik9000

nik9000 commented Apr 19, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing @estolfo! This is a big help!

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