peartherapeutics / bitrise-aws-device-farm-runner

Custom Bitrise step to run tests on aws device farm
MIT License
10 stars 21 forks source link

Upload both app & test package in one step #20

Closed jcarr-sailthru closed 7 years ago

jcarr-sailthru commented 7 years ago

Currently the test packages for Devicefarm have to be uploaded via bitrise-aws-device-farm-file-deploy and the application package is uploaded via this bitrise-aws-device-farm-runner step.

However this has two main problems:

  1. It opens the build process up to a race condition. If two tests are running in parallel with the same test package file name, you could end up in a situation where the test package from one build ends up getting used for both builds.

  2. Usage of this step is quite confusing for new users due to how the uploads are handled differently.

This commit fixes the first problem and hopefully improves the second problem, by having both the test package and application package uploaded by the runner step and the use of their ARNs to prevent any contamination of build jobs with the files uploaded for another job.

Note that this commit may break things for users - the test_package_name param is now expected to point to a file to be uploaded, which may or may not be true for some users. Possibly we should rename it to test_package_path, but that would then certainly break it for all current users which may be more or less desirable - feedback welcome here.

The old bitrise-aws-device-farm-file-deploy job will also now be irrelevant and should be removed from the build workflow - although it will not cause issues if it does remain other than slower builds due to wasteful uploading.

jcarr-sailthru commented 7 years ago

Fair enough @fadookie. I think the current workflow as-is should be tweaked a bit and instead of passing the name of the file to use, the ARN of the file from the previous run should get passed through - but I understand your comment about your scenario of the different test vs app repos.

re EXTERNAL_DATA - Am I correct in my understanding that there would only ever be one external package per device farm run?

Seems like the ideal solution would be to have all three uploads supported in the one step (this one) but have the ability to supply an existing ARN via environmentals, so use cases like yours where your tests are in different repos to the app could be supported.

Thoughts?

fadookie commented 7 years ago

@jcarr-sailthru I believe there would only be one EXTERNAL_DATA per run, yes. I don't think it's worth spending too much thought on that until we have somebody who needs that functionality though, I've never used it.

The problem with requiring a specific test package ARN is that I can't think of a way to pass the ARN of the most recently uploaded test to a completely separate bitrise job, and it will change very frequently (whenever we update the tests) so it's not practical to manually specify the ARN in an env var. Thus the existing solution of doing a lookup of the most recent test package with a given filename.

How about the following solution:

How does that sound?

fadookie commented 7 years ago

@jcarr-sailthru Hey, it's been almost a month. Do you plan to address my change request anytime soon, or should I close this PR and a new one can be opened later? In any case thanks for your contributions so far, they are much appreciated.

jcarr-sailthru commented 7 years ago

hi @fadookie - sorry been on leave. I think your suggestions make sense, but I probably won't have time to tackle it anytime soon - so if you want to close this PR for now, I'm happy with that.

fadookie commented 7 years ago

@jcarr-sailthru Thanks for the update. Will close for now but looking forward to future revisions if you can find time!