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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not change or generate README.md after running rspec #8629

Merged
merged 6 commits into from Apr 1, 2017

Conversation

nafu
Copy link
Collaborator

@nafu nafu commented Mar 23, 2017

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Description

Do not change or generate README.md after running rspec.
If anybody knows better way for handling this, please feedback to this PR. 馃檹

Motivation and Context

After running rspec, it generates some README.md on my machine that is not needed without this patch.

Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

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

I noticed those files too, and thought it's related to my machine somehow. Instead of adding those workarounds, can we just not generate those files at all instead? Maybe just mock the method that writes out the file?

@@ -50,6 +50,8 @@
expect do
Fastlane::CLIToolsDistributor.take_off
end.to raise_error(SystemExit)

`git checkout fastlane/README.md`
Copy link
Member

Choose a reason for hiding this comment

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

That seems like a hack, can we just not generate the file in the first place instead?

allow(FastlaneCore::FastlaneFolder).to receive(:path).and_return(fastlane_folder)
end

after(:all) do
FileUtils.rm(File.join(fastlane_folder, 'README.md'))
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -70,7 +72,11 @@ def ensure_dot_env_value_from_folders(folders, envs, expected_values)

describe "successfull init" do
before do
allow(FastlaneCore::FastlaneFolder).to receive(:path).and_return(File.absolute_path('./fastlane/spec/fixtures/fastfiles/'))
allow(FastlaneCore::FastlaneFolder).to receive(:path).and_return(fastlane_folder)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -5,6 +5,8 @@
ENV['DOTENV'] = nil
fastlane_folder = File.absolute_path('./fastlane/spec/fixtures/dotenvs/withFastfiles/parentonly/fastlane')
allow(FastlaneCore::FastlaneFolder).to receive(:path).and_return(fastlane_folder)
allow(FastlaneCore::Env).to receive(:truthy?).and_return(:default)
allow(FastlaneCore::Env).to receive(:truthy?).with('FASTLANE_SKIP_DOCS').and_return(true)
Copy link
Member

Choose a reason for hiding this comment

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

Should we set this for all tests inside the spec_helper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes 馃憤

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@KrauseFx Uhh, no after all. I tried to define these in spec_helper, and it had other problems to cause test failure.

@@ -43,6 +43,8 @@
end

it "checks for updates even if the lane has an error" do
allow(FastlaneCore::Env).to receive(:truthy?).and_return(:default)
allow(FastlaneCore::Env).to receive(:truthy?).with('FASTLANE_SKIP_DOCS').and_return(true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is 3 duplication codes, so it is better to define helper in fastlane_core/spec/spec_helper.rb or fastlane/spec/spec_helper.rb?

Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

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

I think this solution is strange.

You should not stub this environment variables.

It's natural to skip executing DocsGenerator.run in testing.

Fastlane::DocsGenerator.run(ff) unless FastlaneCore::Env.truthy?("FASTLANE_SKIP_DOCS")

Fastlane::DocsGenerator.run(ff) if !Helper.test? && !FastlaneCore::Env.truthy?("FASTLANE_SKIP_DOCS")

@nafu
Copy link
Collaborator Author

nafu commented Mar 28, 2017

Hmm, CircleCI select Xcode 7.0 馃槗

image

https://circleci.com/gh/fastlane/fastlane/8447

@nafu
Copy link
Collaborator Author

nafu commented Mar 29, 2017

@KrauseFx @giginet
I addressed the feedbacks, could you review again?

Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

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

Looks great 馃憤 Thanks @nafu

@giginet
Copy link
Collaborator

giginet commented Mar 30, 2017

@nafu Sorry for my late reply.
You can merge it 馃槃

@nafu
Copy link
Collaborator Author

nafu commented Apr 1, 2017

@KrauseFx @giginet Thanks 馃殌

@nafu nafu merged commit 068aecb into fastlane:master Apr 1, 2017
@nafu nafu deleted the remove-readme-after-run-rspec branch April 1, 2017 05:44
@fastlane-bot
Copy link

Hey @nafu 馃憢

Thank you for your contribution to fastlane and congrats on getting this pull request merged 馃帀
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 馃憤
We'll notify you once we shipped a new release with your changes 馃殌

@fastlane-bot
Copy link

Congratulations! 馃帀 This was released as part of fastlane 2.25.0 馃殌

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants