Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORDA-2345: Simplified TestCordapp to make it inline with the recent CorDapp versioning changes #4434

Merged
merged 5 commits into from Dec 20, 2018

Conversation

@shamsasari
Copy link
Member

shamsasari commented Dec 18, 2018

TestCordapp has now two implementations to clearly separate the two use cases it has in the Corda repo:

  • TestCordappImpl which implements the revised public API of TestCordapp; namely that a TestCordapp instance references a real CorDapp jar on the classpath. This is either an external dependency jar in which case it’s taken as is and given to the node, or it’s a local gradle project in which case it’s compiled using the gradle jar task to generate the CorDapp jar. This approach means the jar has all the original CorDapp versioning information, which is important that it’s correct when testing. To this end, TestCordapp only needs to expose the ability to specify the app’s config. All the remaining properties have moved to CustomCordapp.

  • CustomCordapp for creating arbitrary custom CorDapps, including specifying the jar’s MANIFEST values. This is internal API and only used for testing the platform. Technically this shouldn’t implement TestCordapp but does so to reduce the complexity of the driver and mock network.

@shamsasari shamsasari force-pushed the shams-testcordapp-cleanup branch 8 times, most recently from 221d5b6 to e8cfa8f Dec 18, 2018
…CorDapp versioning changes

TestCordapp has now two implementations to clearly separate the two use cases it has in the Corda repo:

* TestCordappImpl which implements the revised public API of TestCordapp; namely that a TestCordapp instance references a real CorDapp jar on the classpath. This is either an external dependency jar in which case it’s taken as is and given to the node, or it’s a local gradle project in which case it’s compiled using the gradle “jar” task to generate the CorDapp jar. This approach means the jar has all the original CorDapp versioning information, which is important that it’s correct when testing. To this end, TestCordapp only needs to expose the ability to specify the app’s config. All the remaining properties have moved to CustomCordapp.

* CustomCordapp for creating arbitrary custom CorDapps, including specifying the jar’s MANIFEST values. This is internal API and only used for testing the platform. Technically this shouldn’t implement TestCordapp but does so to reduce the complexity of the driver and mock network.
@shamsasari shamsasari force-pushed the shams-testcordapp-cleanup branch from e8cfa8f to 06bc761 Dec 19, 2018
@shamsasari shamsasari changed the title Shams testcordapp cleanup CORDA-2345: Simplified TestCordapp to make it inline with the recent CorDapp versioning changes Dec 19, 2018
@shamsasari shamsasari requested review from josecoll, tudor-malene and sollecitom and removed request for tudor-malene Dec 19, 2018
Copy link
Contributor

sollecitom left a comment

Some comments.

* disparate classes. The CorDapp metadata that's present in the MANIFEST can also be tailored.
*/
data class CustomCordapp(
val packages: Set<String>,

This comment has been minimized.

Copy link
@sollecitom

sollecitom Dec 19, 2018

Contributor

Mind adding default to empty set here?

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

I could but then it would make the default c'tor invalid (see the init block). We have the cordappWithPackages and cordappForClasses helper methods for this.

private val cache = ConcurrentHashMap<CustomCordapp, Path>()

init {
val buildDir = Paths.get("build").toAbsolutePath()

This comment has been minimized.

Copy link
@sollecitom

sollecitom Dec 19, 2018

Contributor

Does this work from both idea and gradle?

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

Yes, tested on both

defaultJarSignerDirectory = buildDir / "jar-signer" / timeDirName
}

fun getJarFile(cordapp: CustomCordapp): Path {

This comment has been minimized.

Copy link
@sollecitom

sollecitom Dec 19, 2018

Contributor

We should probably pass the directory through the signature.

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

That will complicate things without much benefit I think. The purpose of the directory is a cache, so for max benefit we'd want to use the same directory per JVM instance.

* Represents a completely custom CorDapp comprising of resources taken from packages on the existing classpath, even including individual
* disparate classes. The CorDapp metadata that's present in the MANIFEST can also be tailored.
*/
data class CustomCordapp(

This comment has been minimized.

Copy link
@sollecitom

sollecitom Dec 19, 2018

Contributor

This should also allow resources, like the original.

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

I didn't see any uses of this, though it is a good idea. Perhaps add it when we need it.

@sollecitom

This comment has been minimized.

Copy link
Contributor

sollecitom commented Dec 19, 2018

OK then, as long as it's internal.

docs/source/upgrade-notes.rst Outdated Show resolved Hide resolved
* @see DriverParameters.cordappsForAllNodes
* @see NodeParameters.additionalCordapps
* @see MockNetworkParameters.cordappsForAllNodes
* @see MockNodeParameters.additionalCordapps
*/
@DoNotImplement
interface TestCordapp {

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

personally I prefer to the withXXX DSL methods over copy()

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

I'll add a withConfig for the config option.

Otherwise for the other properties in the internal CustomCordapp I'd rather not add wither methods when our Kotlin tests can benefit from the default parameters in copy.

* Represents a completely custom CorDapp comprising of resources taken from packages on the existing classpath, even including individual
* disparate classes. The CorDapp metadata that's present in the MANIFEST can also be tailored.
*/
data class CustomCordapp(

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

why introduce another type?

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

For the ability to create truly custom CorDapp jars, which some of our tests require. I've explained all in the Kdocs for this class and in this PR. Let me know if this hasn't been explained properly.

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

OK but the name kind of suggests its public rather than internal.
after all it extends TestCordappInternal.

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

It's in a .internal. package so it should be clear it's internal. I could rename it to CustomInteralCordapp, but it's longer, and as you say, it's easy to see that it implements TestCordappInternal.

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

OK - fair enough.
We may expose this publicly if there is demand later on for more advanced testing.

val jmxPolicy: JmxPolicy,
val notarySpecs: List<NotarySpec>,
val compatibilityZone: CompatibilityZoneParams?,
val networkParameters: NetworkParameters,
val notaryCustomOverrides: Map<String, Any?>,
val inMemoryDB: Boolean,
val cordappsForAllNodes: Collection<TestCordapp>,
val signCordapps: Boolean

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

why has this been removed?

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

We had two ways to sign a jar previously: this parameter on the driver which would sign all jars, and on the test cordapp instance. The way this was implemented made it hard to follow, plus it's a bit unnecessary when you can specify signing on the CustomCordapp level.

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

hmmm .... I think signing is something that 3rd party developers may with to use in the Public API.

This comment has been minimized.

Copy link
@shamsasari

shamsasari Dec 19, 2018

Author Member

This feature was not previously public, or rather was not designed to be public by @szymonsztuka in its previous state (after having spoken with him). You would need to flesh it out completely with keystore aliases, passwords, etc. Right now it's all hardcoded.

Secondly, I don't think developers would need signing capabilities anyway. Whether a CorDapp is signed or not is self-evident in the dependency jar file that's pulled in, or it's specified in the CorDapp's project build.gradle (if it's a local build). It either case the app developer doesn't need to tell the driver or mock network to sign the jar, as it's already signed.

This comment has been minimized.

Copy link
@josecoll

josecoll Dec 19, 2018

Contributor

Lets hope so ... assuming they are starting with a clean sheet Corda 4.
Otherwise they may wish to test Upgradability of their Corda 3 CorDapp (unsigned) to Corda 4 (signed). I guess we can always expose the internal CustomCordapp if there is a demand for more advanced testing.

@shamsasari

This comment has been minimized.

Copy link
Member Author

shamsasari commented Dec 19, 2018

I'm going to wait for #4422 to be merged first.

Copy link
Contributor

josecoll left a comment

LGTM!

@shamsasari shamsasari merged commit 830959c into master Dec 20, 2018
5 checks passed
5 checks passed
API Stability (Pull Requests) TeamCity build finished
Details
Build Pull Requests (Tests) TeamCity build finished
Details
Build Pull Requests - WIP (Docs) TeamCity build finished
Details
Integration Tests (Pull Requests) TeamCity build finished
Details
Unit Tests (Pull Requests) TeamCity build finished
Details
@shamsasari shamsasari deleted the shams-testcordapp-cleanup branch Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.