Pass fixture instance into example listeners #264
Conversation
| @@ -78,7 +78,7 @@ public void override(Resource resource) throws IOException { | |||
| public ResultSummary process(Fixture fixture) throws IOException { | |||
| SummarizingResultRecorder resultRecorder = new SummarizingResultRecorder(); | |||
| resultRecorder.setSpecificationDescription(fixture.getSpecificationDescription()); | |||
| getSpecification(fixture).process(evaluatorFactory.createEvaluator(fixture.getFixtureObject()), resultRecorder); | |||
| getSpecification(fixture).process(evaluatorFactory.createEvaluator(fixture.getFixtureObject()), resultRecorder, fixture); | |||
There was a problem hiding this comment.
It seems odd having to pass the fixture to two calls here on the same line.
There was a problem hiding this comment.
Good spot, SpecificationByExample holds the fixture, so I'll remove it from the process method.
There was a problem hiding this comment.
I've modified the handling so getSpecification() no longer requires a fixture
| @@ -11,26 +11,26 @@ public AbstractCommandDecorator(Command command) { | |||
| this.command = command; | |||
| } | |||
|
|
|||
| public void setUp(final CommandCall commandCall, final Evaluator evaluator, final ResultRecorder resultRecorder) { | |||
| public void setUp(final CommandCall commandCall, final Evaluator evaluator, final ResultRecorder resultRecorder, final Fixture fixture) { | |||
| process(commandCall, evaluator, resultRecorder, new Runnable() { | |||
| public void run() { | |||
There was a problem hiding this comment.
Have we moved to the version of Java that has Lambdas yet? If so, the whole 'new Runnable' thing isn't needed anymore.
There was a problem hiding this comment.
Not yet, still supporting JDK 7 currently (and checking with openjdk7 on Travis). Moving up can't be far away though.
|
The PR looks OK. However, 3/4ths of it (or more) is just adding a new parameter to lots of existing methods. This is a code smell to me - and indicates we have need of a Data object of some type to encapsulate all the arguments - and mostly they are "CommandCell, Evaluator, ResultRecorder, and Fixture". This indicates some sort of "RuntimeContext" argument. It might be sensible to do this refactor now because we're breaking the API anyway - then we can add new things to the RuntimeContext object later and not break the API as badly. (RuntimeContext is a bad name - but it's a starting point) |
… specification types
* origin/master: Review Change report URL before running specification in order to guarantee correct URLs in test report
Improve Fixture handling
* origin/master: Add SpecCallingFailFastExample spec to Example spec Create new specs for href updates on run spec
…to before-examples * 'before-examples' of github.com:concordion/concordion:
* origin/master: Prevent calls to @BeforeExample and @AfterExample annotated methods for ignored examples Fixed (jdk6 related) bug Introduced abstraction to the underlying example element to reduce coupling with DOM API Removed dependency to JUnit @ignore annotation Update .travis.yml [Gradle Release Plugin] - new version commit: '2.1.4'. Updated old task in build.gradle Updated gradle wrapper distributionUrl for Java 7 on Travis Updated gradlew and git-publish plugin Changed ExampleFilter to ImplementationStatusModifier and changed SKIPPED to IGNORED Provide an extension point to allow filtering/skipping specification examples Added JDK10 to Travis
* master: Added extra properties to MANIFEST.MF Updated website URL to use https Updated Travis to use oraclejdk10
Please don't merge this PR yet while we look at the impact on existing extensions.
This will impact at least the ScreenshotExtension (which implements
Command) and the ParallelRunExtension (which has an outer example listener).To ensure that the correct fixture instance is utilised in methods annotated with
@BeforeExampleand@AfterExample, pass the Fixture object intoExampleEvents, which necessitates passing it throughExampleCommands.Fixes #246 and #262.