Samples composite build #440

assaflei · 2020-04-13T21:53:17Z

Started following instructions
How do I test the setup?

In misc\tools\atrium-samples-test\build.gradle I simply copied the file from atrium-bc-test and renamed jvm to js hope its enough
I think that this comment in this file is redundand, is it?
//TODO tests are now mostly in the common module. However, we cannot unzip this to the jvm project but // require a common module for it. Probably easiest to introduce a second bc-test which is an MPP project // add(testJarSources, "$groupId:atrium-api-$apiName-common:$oldVersion:testsources") { // exclude group: '*' // }

I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

robstoll · 2020-04-14T05:36:45Z

I see, the first point in the issue lacks a possibility to test the setup. I have updated the description; merged the first and the last point.
atrium-bc-test was only meant as guideline but maybe it is more confusing than helping as it does very specific things -- e.g. copy old versions of Atrium into src/old, you won't do anything like that in your module. Maybe it is better to do it from scratch, i.e. only follow the guideline of gradle.

assaflei · 2020-04-24T11:27:36Z

got back to this after some time...

tried starting it
it seems like the js modules are dependent on fluent-en_GB-js build
it looks like a tight coupling that breaks the standalone requirement
am I correct here?
currently the build containing jasmine fails with the message: Project with path 'ch.tutteli.atrium:atrium-fluent-en_GB-js:0.11.0' could not be found in project ':jasmine'.

what do you think?

assaflei · 2020-04-24T12:22:08Z

I think I managed to execute all tests eventually and its successful

robstoll

@assaflei very unfortunate but we cannot use gradles' composite build. I did not have a look at it yet but saw now that it does not work:

When a configuration other than default is published.

Builds using these features function incorrectly when included in a composite build. We plan to improve this in the future.

So yeah... we'll have to workaround it, implement our own composite build functionality.
Let me know if you still want to tackle this problem. I quickly outline an alternative solution: some of the steps which will be required:

publish Atrium to a local maven repository (will require that we tweak the publishing a bit as it usually requires gpg to be setup)
replace the atrium-version in the samples with the current snapshot version
include mavenLocal() into the samples
build the modified samples

I suggest that I will do the first step. In case you are interested, you could to steps 2 - 4. Let me know and I can give more information.

misc/tools/atrium-samples-test/.gitignore

samples/js/jasmine/build.gradle

samples/js/jasmine/settings.gradle

samples/js/mocha/build.gradle

samples/js/mocha/settings.gradle

robstoll · 2020-04-24T13:36:37Z

misc/tools/atrium-samples-test/build.gradle

@@ -0,0 +1,7 @@
+task run {
+    dependsOn gradle.includedBuild('junit5').task(':test')


I am afraid but I made a wrong assumption. I did not have a look at how composite builds work in gradle but they work the opposite way. I.e. the project which depends on another needs to do the includeBuild. Which means, we don't need a atrium-samples-test at all because not atrium has a dependency but the opposite. Thus remove the whole atrium-samples-test again. Instead it is enough if we add the --include-build to the CI-definitions (.travis.yml and java-windows.yml). So basically duplicate the existing build-steps/jobs and add --include-build. for instance, for mpp in java-windows.yml:

- name: Build sample mpp with current Atrium run: samples\multiplatform\gradlew -p samples\multiplatform build --include-build ../../../

Wait wait... maybe gradle is wrong (let's hope). Maybe this approach works but you surely need to include Atrium as well (and make atrium-samples-test an independent project entirely, i.e. do not add it to settings.gradle in the root of the project). In order to test if it works, I advice you to change apis/fluent-en_GB/atrium-api-fluent-en_GB-common/src/main/kotlin/ch/tutteli/atrium/api/fluent/en_GB/anyAssertions.kt use the following for toBe

fun <T> Expect<T>.toBe(expected: T) = addAssertion(ExpectImpl.any.notToBe(this, expected))

if the composite build works, then the tests should be executed and fail.

what is the meaning of ../../../?
where are we pointing to?
the include build was required in case we wanted a composite build, if we wont use it than why use --include-build?

Wait wait... maybe gradle is wrong (let's hope). Maybe this approach works but you surely need to include Atrium as well (and make atrium-samples-test an independent project entirely, i.e. do not add it to settings.gradle in the root of the project). In order to test if it works, I advice you to change apis/fluent-en_GB/atrium-api-fluent-en_GB-common/src/main/kotlin/ch/tutteli/atrium/api/fluent/en_GB/anyAssertions.kt use the following for toBe

fun <T> Expect<T>.toBe(expected: T) = addAssertion(ExpectImpl.any.notToBe(this, expected))

if the composite build works, then the tests should be executed and fail.

I missed this comment...
Should we revert?

the include build was required in case we wanted a composite build, if we wont use it than why use --include-build?

Sorry for the confusion, that comment was before I saw the restriction of composite builds.

But to answer your other question

what is the meaning of ../../../?

note the parameter -p in

samples\multiplatform\gradlew -p samples\multiplatform build --include-build ../../../

so ../../../ is pointing back to the root of the project.

arent we running from the root already?
samples... looks like reference from the root

-p specifies the working directory for gradle and --include-build is relative to this working directory.

understood
so leave the yml files without change or should I try and add the include-build param?

I'll see if I can build up on your current work, tweaking gradle a bit so that we still can use composite build. But this would require atrium-samples-test. So for the moment you can wait until I get back to you.

What you can do already in the meantime is:

squash your commits, rebase on master and force push

assaflei · 2020-04-24T14:45:36Z

Lets go for the next steps as you require, I'll learn whats required
Update me when the env is ready

robstoll · 2020-04-24T14:47:05Z

Lets go for the next steps as you require, I'll learn whats required

Cool, I'll look a bit more into composite builds and let you know once pre-work is done.

robstoll · 2020-04-24T15:32:12Z

Nice, I have a working workaround, so you can proceed:

add the following to settings.gradle:

 includeBuild('../../../') {
     dependencySubstitution {
         gradle.includedBuild('atrium').getAvailableModules().collect { pair -> pair.right.projectName }
             .findAll { it.endsWith("-jvm") }
             .forEach { projectName ->
                 def withoutSuffix = projectName.substring(0, projectName.length() - 4)
                 substitute module("ch.tutteli.atrium:$withoutSuffix") with project(":$projectName")
             }
     }
 }

in build.gradle, change the dependsOn and always depend on :build (instead of test etc.)
change .travis.yml and java-windows.yml remove steps/jobs for samples and step/job for atrium-samples-test instead.

I can also merge your changes into a separate branch and push my changes, if you prefer -- I mean, you don't have to do double work really. Unless you want to do this on your own I am going to merge this into a separate branch.

assaflei · 2020-04-24T15:41:21Z

which settings.gradle do you mean? at the root? or of the js and jvm projects we address?
also just point me to the branch you used so i could merge changes into my own branch

robstoll · 2020-04-24T15:42:00Z

I meant settings.gradle of atrium-samples-test and build.gradle of atrium-samples-test

robstoll · 2020-04-24T15:44:53Z

You can see my changes here (note that it is not based on your latest commits):
https://github.com/robstoll/atrium/tree/pull/440
1ba255a

assaflei · 2020-04-24T15:53:35Z

updated as required, whats next?

robstoll · 2020-04-24T15:56:05Z

squash your commits, rebase on master and force push
I'll review in the meantime

robstoll · 2020-04-24T15:57:44Z

Looks good, the only thing left is that you have merge conflicts you need to resolve (let me know in case you need help with that).

assaflei · 2020-04-24T16:00:02Z

squash your commits, rebase on master and force push
I'll review in the meantime

trying to understand this first... never done before

robstoll · 2020-04-24T16:06:29Z

I usually do it as follows (from top of my head, errors included 😉)
get first commit hash (e.g. from here https://github.com/robstoll/atrium/pull/440/commits)
then:

git reset --soft f6e09c69a6cc4b3561cc23d3ba7d8fd2e18b989d

takes you back to f6e09c6 and adds all modification to the staged changes (do a git status to see that)
next we want to include those changes in the current commit

git commit --amend -m "composite build, make mocha/jasmine independent"

now you have one single commit (see git log) which we want to put on top of upstream/master
(add it in case you don't have it yet with git remote add upstream https://github.com/robstoll/atrium.git and)

git fetch upstream master
git rebase upstream/master

Here you will get a conflict in settings.gradle.
Open settings.gradle and fix it

git add settings.gradle
git rebase --continue

now your commit should be on master

git push -f

force push your branch, i.e. let the branch-pointer switch from the previous commit hash to the new one

assaflei · 2020-04-25T18:47:04Z

Followed your instructions and its all good, thank you!
What is required next?

settings.gradle

robstoll · 2020-04-25T18:53:24Z

I think it should be good now but let's wait to see if the build is green.

codecov-io · 2020-04-25T19:00:18Z

Codecov Report

Merging #440 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             master     #440    +/-   ##
==========================================
  Coverage     87.84%   87.84%            
+ Complexity      125        1   -124     
==========================================
  Files           712      712            
  Lines          6152     6152            
  Branches        241      241            
==========================================
  Hits           5404     5404            
  Misses          698      698            
  Partials         50       50

Flag	Coverage Δ	Complexity Δ
#bbc	`72.37% <ø> (ø)`	`0.00 <ø> (ø)`
#bc	`61.62% <ø> (ø)`	`0.00 <ø> (ø)`
#current	`71.02% <ø> (-11.41%)`	`152.00 <ø> (ø)`
#current_windows	`70.00% <ø> (ø)`	`125.00 <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a064a0...f4fb850. Read the comment docs.

robstoll · 2020-04-25T19:07:39Z

@assaflei you need to give gradlew in atrium-samples-tests execution rights, the build failed with

/home/travis/.travis/functions: line 109: misc/tools/atrium-samples-test/gradlew: Permission denied

this can be done as follows:

cd misc/tools/atrium-samples-test
chmod +x gradlew

commit and push afterwards

assaflei · 2020-04-25T19:19:50Z

add this to this file gradlew?

robstoll · 2020-04-25T19:23:46Z

No worries, I guess you are a Windows user and it might be not trivial to do it there. I'll do it.

assaflei · 2020-04-25T19:25:54Z

So is this issue closed?

robstoll · 2020-04-25T19:26:22Z

I created a new PR: #471 let's see if the build passes now. If it does then everything is done 👍

assaflei · 2020-04-25T19:38:12Z

@assaflei you need to give gradlew in atrium-samples-tests execution rights, the build failed with
/home/travis/.travis/functions: line 109: misc/tools/atrium-samples-test/gradlew: Permission denied
this can be done as follows:
cd misc/tools/atrium-samples-test
chmod +x gradlew
commit and push afterwards

just to understand the context, you wanted to add the commands to gradlew file in order for them to run as part of the travis build?

robstoll · 2020-04-25T19:43:44Z

travis tries to execute gradlew as part of the build (that's what you have set up: misc/tools/atrium-samples-test/gradlew -p ./misc/tools/atrium-samples-test build), In order that it can do that, the file needs execution rights; otherwise travis fails because it is not allowed to execute it.
It's a bit unfortunate, but you cannot see the change I did here:
81881bc
but its basically setting the execution flag with chmod +x gradlew

robstoll · 2020-04-25T19:47:05Z

But you can see it when you do: git show 81881bc

commit 81881bc6329f521d509d2be886f1beadac4d8398 (origin/#245-samples-composite-build, #245-samples-composite-build)
Author: Robert Stoll <rstoll@tutteli.ch>
Date:   Sat Apr 25 21:25:13 2020 +0200

    add execution rights to misc/tools/atrium-samples-test/gradlew

diff --git a/misc/tools/atrium-samples-test/gradlew b/misc/tools/atrium-samples-test/gradlew
old mode 100644
new mode 100755

See, the mode of the file changed from 644 to 755. For more information about the modes: https://en.wikipedia.org/wiki/Modes_(Unix)

assaflei · 2020-04-25T19:56:54Z

I didnt see any line of code related to this change
You just changed the setting on your own machine and the file was marked and committed with the new permission?

robstoll · 2020-04-25T20:02:40Z

Exactly, I changed the file mode, committed that change as you would commit any other change and pushed it. GitHub (and many IDEs) are lacking the feature of showing file mode changes. That's unfortunate but in the end git is the only truth 😉

robstoll · 2020-04-25T20:04:48Z

btw. the other PR passed, thanks for your contribution 👍

There is #470 which deals with a sample project, so kind of in the same context. Would you like to tackle this one? Or perhaps more Kotlin related? For instance 460 would be a good start.

assaflei · 2020-04-25T20:07:19Z

throw whatever is more important :)
this is a great learning experience for me

robstoll · 2020-04-25T20:23:06Z

In this case, it would be great if you could tackle #167

ps: I have sent you a contributor invitation

assaflei requested a review from robstoll as a code owner April 13, 2020 21:53

robstoll reviewed Apr 24, 2020

View reviewed changes

composite build, make mocha/jasmine independent

051fb05

assaflei force-pushed the #245-samples-build branch from 55bcf97 to 051fb05 Compare April 25, 2020 18:46

robstoll reviewed Apr 25, 2020

View reviewed changes

settings.gradle Outdated Show resolved Hide resolved

removed samples block from settings.gradle

f4fb850

robstoll approved these changes Apr 25, 2020

View reviewed changes

robstoll changed the base branch from master to #245-samples-composite-build April 25, 2020 19:23

robstoll merged commit 65aae37 into robstoll:#245-samples-composite-build Apr 25, 2020

robstoll changed the title ~~[WIP] Samples composite build~~ Samples composite build Apr 25, 2020

assaflei deleted the #245-samples-build branch April 25, 2020 20:07

assaflei mentioned this pull request May 15, 2020

merge jdk8 extensions to jvm #497

Merged

robstoll mentioned this pull request Jul 13, 2020

#477 deprecate defaultTranslationOf(IgnoringCase) #542

Closed

		@@ -0,0 +1,7 @@
		task run {
		dependsOn gradle.includedBuild('junit5').task(':test')

Samples composite build #440

Samples composite build #440

Conversation

assaflei commented Apr 13, 2020

robstoll commented Apr 14, 2020

assaflei commented Apr 24, 2020

assaflei commented Apr 24, 2020

robstoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assaflei commented Apr 24, 2020

robstoll commented Apr 24, 2020 • edited Loading

robstoll commented Apr 24, 2020 • edited Loading

assaflei commented Apr 24, 2020

robstoll commented Apr 24, 2020 • edited Loading

robstoll commented Apr 24, 2020

assaflei commented Apr 24, 2020

robstoll commented Apr 24, 2020

robstoll commented Apr 24, 2020 • edited Loading

assaflei commented Apr 24, 2020

robstoll commented Apr 24, 2020 • edited Loading

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020 • edited Loading

codecov-io commented Apr 25, 2020 • edited Loading

Codecov Report

robstoll commented Apr 25, 2020 • edited Loading

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020

robstoll commented Apr 25, 2020

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020

robstoll commented Apr 25, 2020

assaflei commented Apr 25, 2020

robstoll commented Apr 25, 2020 • edited Loading

robstoll commented Apr 24, 2020 •

edited

Loading

robstoll commented Apr 24, 2020 •

edited

Loading

robstoll commented Apr 24, 2020 •

edited

Loading

robstoll commented Apr 24, 2020 •

edited

Loading

robstoll commented Apr 24, 2020 •

edited

Loading

robstoll commented Apr 25, 2020 •

edited

Loading

codecov-io commented Apr 25, 2020 •

edited

Loading

robstoll commented Apr 25, 2020 •

edited

Loading

robstoll commented Apr 25, 2020 •

edited

Loading