Skip to content

WIP: Changeover most DFDLTestSuite invocations to Runner#430

Closed
IanCarlsonOwl wants to merge 2 commits intoapache:mainfrom
IanCarlsonOwl:daffodil-1300
Closed

WIP: Changeover most DFDLTestSuite invocations to Runner#430
IanCarlsonOwl wants to merge 2 commits intoapache:mainfrom
IanCarlsonOwl:daffodil-1300

Conversation

@IanCarlsonOwl
Copy link
Copy Markdown
Contributor

Tweak the interface to the Runner object, as well as
creating a RunnerOpts object to handle the multiple
combinations of parameters used by various tests without
compromising the ability to default parameters
not under test.

DAFFODIL-1300

@IanCarlsonOwl
Copy link
Copy Markdown
Contributor Author

There are a number of calls to DFDLTestSuite still outstanding, but I wanted to get this much in front of others for a sanity-check, and to ensure it builds on all of our target systems.


def apply(elem: scala.xml.Elem, options: RunnerOpts): Runner =
new Runner(elem, options)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm concerned that this could break existing Runners not in the daffodil repo. Other schema repos use this Runner for testing and they could currently provide options which would no longer compile once this change is made. I'm wondering if it makes sense to keep an apply the old dir/file apply method with all options defaulted and creates the new RunnerOpts using those values? We can deprecate that apply method so other repos will still work but get a message to fix them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would second this. We need the original Runner factory method, as there are dozens of DFDL schemas we know of, and possibly others we don't know of or have access to, which are using the existing Runner factory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In commit b123d9f I restored the old apply methods.

runnerDelimited.reset
runnerMD.reset
runnerMD_NV.reset
runnerBB.reset
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we are modifying so many test files do we also want to add the shutDown method to reset the runners? I think many of them are missing.

I'm wondering if a convenient way to do this would be to add a new mixin (i.e. DaffodilTestSuite) that implements shutDown and uses reflection to find any Runner members and automatically call reset on them?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would call this mixin AllRunnersAutoShutdownMixin.

Copy link
Copy Markdown
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable approach to me 👍

Copy link
Copy Markdown
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

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

I agree with Steve's suggestions, especially the one to define a trait DFDLTestSuite with an @afterclass shutDown method which XXXTests objects can extend to automatically call reset() on any member fields matching the type DFDLTestSuite. Overall, your approach looks good.

Tweak the interface to the Runner object, as well as
creating a RunnerOpts object to handle the multiple
combinations of parameters used by various tests without
compromising the ability to default parameters
not under test.

DAFFODIL-1300
@mbeckerle
Copy link
Copy Markdown
Contributor

Closed as inactive. Ticket DAFFODIL-1300 is closed. If there is other functionality here being worked on a new JIRA ticket is needed.

@mbeckerle mbeckerle closed this Feb 6, 2024
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.

4 participants