Skip to content

Conversation

@proski
Copy link

@proski proski commented Jun 12, 2019

*  StashRepository: Merge init() into the constructor
   
   Jenkins documentation doesn't guarantee that start() would not be called
   after run() for a trigger.
   
   While it doesn't happen in freestyle projects, Jenkins would actually
   call start() after run() on the trigger in a pipeline project.
   
   That would create a new StashRepository object that would not have the
   StashApiClient object initialized. When the build completes, it would try
   to post the build result as a PR comment. That would throw an exception.
   
   To prevent it, remove the init() method. Move StashApiClient
   initialization inside the StashRepository constructor, so that the
   StashRepository object always has a valid StashApiClient object.
   
   Add WireMock as a test dependency. Check that the newly constructed
   StashRepository object would actually send an HTTP request.

That's one of the commits that make pipeline support possible. Extracted from #69 to help with review.

I realize that constructors are less testable than other methods, but the focus here is on fixing an issue with pipelines with minimal changes.

In the future, we can use factory pattern or generics to improve testability. Also, it might be possible not to replace the StashRepository object on the start() call for the same job. I'm not doing any optimization at this time, I'm making the code more robust.

@proski
Copy link
Author

proski commented Jun 26, 2019

The requested changes have been made. The unit test is using WireMock now.

@proski
Copy link
Author

proski commented Jun 28, 2019

Reworked the PR to keep the constructors short. As a side effect, the diff should be easier to read.

@proski
Copy link
Author

proski commented Jun 28, 2019

Improved the comment for the constructor used in unit tests.

@proski
Copy link
Author

proski commented Jun 29, 2019

Using Java 8 version of WireMock and dynamic HTTP port for testing.

Jenkins documentation doesn't guarantee that start() would not be called
after run() for a trigger.

While it doesn't happen in freestyle projects, Jenkins would actually
call start() after run() on the trigger in a pipeline project.

That would create a new StashRepository object that would not have the
StashApiClient object initialized. When the build completes, it would try
to post the build result as a PR comment. That would throw an exception.

To prevent it, remove the init() method. Move StashApiClient
initialization inside the StashRepository constructor, so that the
StashRepository object always has a valid StashApiClient object.

Add WireMock as a test dependency. Check that the newly constructed
StashRepository object would actually send an HTTP request.
@proski
Copy link
Author

proski commented Jun 30, 2019

Added a verification that WireMock received a GET request.

@jakub-bochenski jakub-bochenski merged commit d93b35a into jenkinsci:master Jul 5, 2019
@proski proski deleted the merge-init branch July 6, 2019 00:25
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