Add tests for TestFlight methods in spaceship and fastlane_core #8962

snatchev · 2017-04-21T21:39:44Z

Future of spaceship testing:

No fixtures: these are too brittle to maintain
Use sinatra app to construct client responses near the test site
More focus on each Unit isolation. Heavier use of mock objects

This is a work in progress

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.

Motivation and Context

Description

fastlane-bot-helper · 2017-04-21T21:41:44Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

hjanuschka · 2017-04-21T21:54:00Z

wow! ❤️ i am in love with the sinatra + json sheme approach! 👍

ohayon · 2017-04-26T19:52:49Z

spaceship/lib/spaceship/test_flight/build.rb

@@ -104,7 +104,7 @@ def self.latest(app_id: nil, platform: nil)
    #
    # Note: this will overwrite any non-saved changes to the object
    #
-    # @return (Spaceceship::Base::DataHash) the raw_data of the build.
+    # @return (Spaceship::Base::DataHash) the raw_data of the build.


ohayon · 2017-04-26T19:58:27Z

spaceship/lib/spaceship/test_flight/client.rb


-    def get_groups(app_id: nil)
+    def put_testinfo(app_id: nil, testinfo: nil)


Do we call this anywhere that it needs to get updated?

Also, just curious why this is changed to know about the model object rather than passing in the fields?

Ah, good question. We currently don't use it. I did use it at one point in a debugging phase to try to fix the problem with our shared test account.

I think it's okay to pass in objects that look like hashes (or at least respond to to_json) as long as they don't need any mutation to result in a successful response. Here, the client doesn't really know anything about testinfo.

Just read the comment above about passing in large objects. Got it!

ohayon · 2017-04-26T20:34:08Z

spaceship/lib/spaceship/test_flight/build.rb

    #
    # @return (Spaceship::TestFlight::Build)
    def self.find(app_id: nil, build_id: nil)
      attrs = client.get_build(app_id: app_id, build_id: build_id)
-      self.new(attrs) if attrs
+      self.new(attrs)


Will this be safe to have this return a Build object without any attributes? Might cause an issue if we are checking for its presence at any point right?

initialize always expects a hash. If nil is passed in, it throws an exception. So far the pattern has been that if an error response is received, an UnexpectedResponse exception is thrown. I think throwing exceptions is consistent in this case.

ohayon · 2017-04-26T20:35:26Z

spaceship/lib/spaceship/test_flight/client.rb


-    def get_groups(app_id: nil)
+    def put_testinfo(app_id: nil, testinfo: nil)


Just read the comment above about passing in large objects. Got it!

ohayon · 2017-04-27T14:13:13Z

spaceship/spec/test_flight/tester_spec.rb

+require 'spec_helper'
+
+describe Spaceship::TestFlight::Tester do
+  let(:tester) { Spaceship::TestFlight::Tester.new }


@mpirri Is this being used?

mfurtak · 2017-04-27T19:28:11Z

fastlane_core/spec/build_watcher_spec.rb

+    it 'waits when a build is still processing' do
+      expect(Spaceship::TestFlight::Build).to receive(:all_processing_builds).and_return([processing_build])
+      expect(Spaceship::TestFlight::Build).to receive(:builds_for_train).and_return([processing_build], [ready_build])
+      expect(FastlaneCore::BuildWatcher).to receive(:sleep).with(10)


suggestion: change this to expect(FastlaneCore::BuildWatcher).to receive(:sleep)

That way, if we ever change how long we want it to sleep, we don't have to go back and update all the tests where we expected a particular number

mfurtak · 2017-04-27T19:29:05Z

spaceship/lib/spaceship/test_flight/base.rb

+    # Essentially, we are making a class-inheritable-accessor as described here:
+    # https://apidock.com/rails/v4.2.7/Class/class_attribute
+    #
+    # TODO[snatchev]: move this into spaceship/base.rb once all the tests pass


Sorry, that was a cryptic comment - are we now in a place where this TODO could be resolved?

mfurtak

This looks awesome - I'd definitely like a run-down on how it all works, though 😉

Might it make sense to update some spaceship developer docs to explain the strategy?

Future of spaceship testing: * No fixtures: these are too brittle to maintain * Use sinatra app to construct client responses near the test site * More focus on each Unit isolation. Heavier use of mock objects This is a work in progress

Also print some helpful info when returning a 404

snatchev · 2017-04-27T20:15:20Z

spaceship/lib/spaceship/test_flight/client.rb


+      platform ||= "ios"


platform will never be nil because of the assertion above. Perhaps we should set platform: 'ios' in the parameter names, or get rid of this.

snatchev · 2017-04-27T20:21:40Z

I agree with @mfurtak about adding some documentation. Where should we add some developer docs?

at the top of some classes
a README in the relevant folder (i.e. spaceship/spec)?
in another designated documentation location?

ohayon · 2017-04-27T20:26:11Z

I think a README.md in the spaceship/spec directory would make sense. @mfurtak thoughts on where the best place for this would be is?

…stlane into snatchev/testing-framework-2

mfurtak · 2017-04-28T00:24:16Z

spaceship/docs/TestFlightTesting.md

 **Examples:**

-Mocked responses:
+At the top of your data model, set the client to be a `mock_client`:


"At the top of your data model spec"?

mfurtak · 2017-04-28T00:24:39Z

spaceship/docs/TestFlightTesting.md

+
+Spaceship wraps various APIs using the following pattern:
+
+A simple `client` and various data models, usually suclassed from a `Base` model (e.g. Spaceship::TestFlight::Base)


"suclassed" -> "subclassed"

mfurtak · 2017-04-28T00:26:02Z

spaceship/docs/TestFlightTesting.md

+
+```ruby
+before do
+  mock_client_response(:get_tester, with: { tester_id: 1 }) do


Where does this method come from? Is there something that has to be required in the spec to make it work?

mfurtak · 2017-04-28T00:28:53Z

spaceship/docs/TestFlightTesting.md

 end
 ```

+The first parameter is the name of the method we are mocking, and `with:` parameter specifies some required parameter to that method. If you don't give it a `with:`, the mock will accept any parameter. The block is the return value of calling `client.get_tester`.


Can this be more than one parameter? If so, is it more clear to say "required parameters" and "any parameters"?

mfurtak

Looking great, thanks for the docs 👍 Just left a few questions - merge when ready

tmm1 · 2017-04-28T20:40:50Z

spaceship/lib/spaceship/test_flight/build.rb

@@ -130,6 +130,10 @@ def export_compliance_missing?
      external_state == BUILD_STATES[:export_compliance_missing]
    end

+    def self.processed?


This should be def processed?, not def self.processed?

NoMethodError: [!] undefined method `processed?' for #<Spaceship::TestFlight::Build:0x007fb066700ee0> gems/fastlane-2.28.6/fastlane_core/lib/fastlane_core/build_watcher.rb:15:in `block in wait_for_build_processing_to_be_complete'

Thanks so much for bringing this to our attention! I can't believe with all the unit tests we added that we missed this...

Anyhow, here is a PR to fix it: #9025
As soon as we can get a 🚢 on it, we will release a new build.

fastlane-bot · 2017-04-29T13:00:33Z

Congratulations! 🎉 This was released as part of fastlane 2.28.6 🚀

googlebot added the cla: yes label Apr 21, 2017

ohayon reviewed Apr 26, 2017

View reviewed changes

ohayon reviewed Apr 27, 2017

View reviewed changes

mpirri changed the title ~~[WIP] Add testing framework for spaceship~~ Add tests for TestFlight methods in spaceship and fastlane_core Apr 27, 2017

mfurtak reviewed Apr 27, 2017

View reviewed changes

mfurtak approved these changes Apr 27, 2017

View reviewed changes

snatchev and others added 19 commits April 27, 2017 15:46

[WIP] Add testing framework for spaceship

6a20c87

Future of spaceship testing: * No fixtures: these are too brittle to maintain * Use sinatra app to construct client responses near the test site * More focus on each Unit isolation. Heavier use of mock objects This is a work in progress

add more test cases, attempt to reset routes after each spec

8695e8d

Reset the routes after each spec.

1538465

Also print some helpful info when returning a 404

add specs for Build collection methods

117d45c

add passing collection spec

b1d1a7b

update more specs

c1207ab

TestFlight::Base.client is inheritable. Update more specs

b6b21cc

add spec for build_trains

e7bc822

refactor and add Spaceship::TestFlight::Group specs

f7f1724

add comment

2cd8c79

use mock client instead of Sinatra app

cdea597

fix group_spec

c92d770

update build_trains to use mock_client

c3f1a4e

add test_info spec

05c7c24

Capital I in TestInfo

37c36d1

rubocop fixes

d006f91

Add tests for groups and builds

67dac96

add test for testers api

2cb6c5b

add tests for testinfo

0a70b6e

Mark Pirri added 5 commits April 27, 2017 15:46

More tests on build watcher

c6243f1

Finish build_watcher tests

7307225

Clean up rubocop stuff

557542a

RuboCop cleanup

094cc8d

Get rid of sleep time requirement

8691c9b

mpirri force-pushed the snatchev/testing-framework-2 branch from 3d8f4c5 to 8691c9b Compare April 27, 2017 19:50

Mark Pirri and others added 3 commits April 27, 2017 16:02

Remove TODOs

42ced98

Get rid of other sleep time requirement

22881aa

Change name of a double

0e4c03a

snatchev commented Apr 27, 2017

View reviewed changes

Make "ios" the default platform.

f02c62b

Mark Pirri and others added 3 commits April 27, 2017 16:56

Add docs for Spaceship::TestFlight tests

adc2f06

Merge branch 'snatchev/testing-framework-2' of github.com:fastlane/fa…

8096abf

…stlane into snatchev/testing-framework-2

Update TestFlightTesting.md

47d143e

mfurtak reviewed Apr 28, 2017

View reviewed changes

mfurtak approved these changes Apr 28, 2017

View reviewed changes

PR feedback

4b42934

ohayon merged commit 1ad43dd into master Apr 28, 2017

ohayon deleted the snatchev/testing-framework-2 branch April 28, 2017 15:00

mpirri mentioned this pull request Apr 28, 2017

Version bump to 2.28.6 #9022

Merged

tmm1 reviewed Apr 28, 2017

View reviewed changes

tmm1 mentioned this pull request Apr 28, 2017

BuildWatcher is broken in 2.28.6 #9024

Closed

fastlane-bot added the status: released label Apr 29, 2017

fastlane locked and limited conversation to collaborators Jul 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for TestFlight methods in spaceship and fastlane_core #8962

Add tests for TestFlight methods in spaceship and fastlane_core #8962

snatchev commented Apr 21, 2017

fastlane-bot-helper commented Apr 21, 2017 •

edited

hjanuschka commented Apr 21, 2017

ohayon Apr 26, 2017

ohayon Apr 26, 2017

snatchev Apr 26, 2017

ohayon Apr 26, 2017

ohayon Apr 26, 2017

snatchev Apr 26, 2017

ohayon Apr 26, 2017

ohayon Apr 27, 2017

mfurtak Apr 27, 2017

mfurtak Apr 27, 2017

mfurtak Apr 27, 2017

mfurtak left a comment

snatchev Apr 27, 2017

snatchev commented Apr 27, 2017

ohayon commented Apr 27, 2017

mfurtak Apr 28, 2017

mfurtak Apr 28, 2017

mfurtak Apr 28, 2017

mfurtak Apr 28, 2017

mfurtak left a comment

tmm1 Apr 28, 2017

mpirri Apr 28, 2017

fastlane-bot commented Apr 29, 2017


		def get_groups(app_id: nil)
		def put_testinfo(app_id: nil, testinfo: nil)


		Spaceship wraps various APIs using the following pattern:

		A simple `client` and various data models, usually suclassed from a `Base` model (e.g. Spaceship::TestFlight::Base)

Add tests for TestFlight methods in spaceship and fastlane_core #8962

Add tests for TestFlight methods in spaceship and fastlane_core #8962

Conversation

snatchev commented Apr 21, 2017

Checklist

Motivation and Context

Description

fastlane-bot-helper commented Apr 21, 2017 • edited

hjanuschka commented Apr 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfurtak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snatchev commented Apr 27, 2017

ohayon commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfurtak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fastlane-bot commented Apr 29, 2017

fastlane-bot-helper commented Apr 21, 2017 •

edited