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

Separate test translation and building from production library #474

Merged
merged 1 commit into from
Oct 1, 2015

Conversation

advayDev1
Copy link
Contributor

No description provided.

@brunobowden
Copy link
Contributor

I'll hold off reviewing it until you ask me.

@advayDev1
Copy link
Contributor Author

yeah hold off for now, i currently have no way of verifying things except pushing to Travis as Xcode 7 is breaking things locally for me

@advayDev1 advayDev1 mentioned this pull request Oct 1, 2015
@advayDev1 advayDev1 changed the title WIP: Separate test translation and building from production library Separate test translation and building from production library Oct 1, 2015
@advayDev1
Copy link
Contributor Author

The next Travis build should pass. But I'm about to lose internet, so if it turns green, please review. @brunobowden

@advayDev1
Copy link
Contributor Author

Well Travis (unit tests + system tests do pass). AppVeyor (windows unit tests) aren't. I'm not certain which of your win32-path utils I should use for this error:
https://ci.appveyor.com/project/madvayApiAccess/j2objc-gradle/build/build27.master-yumdtrni

@brunobowden - could you give me a hint there?

IncrementalTaskInputs incrementalTaskInputs = new IncrementalTaskInputs() {
@Override
boolean isIncremental() { return false }

@Override
void outOfDate(Action<? super InputFileDetails> outOfDateAction) {}
void outOfDate(Action<? super InputFileDetails> outOfDateAction) {
proj.files('src/**/*.java').each { final File file ->
Copy link
Contributor

Choose a reason for hiding this comment

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

The Windows build failed. I think the path may need to be altered to work on Windows. Try and see if the following works:

String filesPath = 'src/**/*.java'
if (Utils.isWindows()) {
    filesPath = 'src\**\*.java'
}
proj.files(filesPath).each { final File file ->
...

Build failure:
https://ci.appveyor.com/project/madvayApiAccess/j2objc-gradle/build/build27.master-yumdtrni

void outOfDate(Action<? super InputFileDetails> outOfDateAction) {}
void outOfDate(Action<? super InputFileDetails> outOfDateAction) {
proj.files(Utils.isWindows() ? 'src\\**\\*.java' : 'src/**/*.java').each {
final File file ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add warning logs (that show up in the logs for the CI builds on all platforms). Have it show the input to proj.files(...) and the value of the iterator on each file. That'll be fairly verbose but I think that'll help us understand what's going wrong here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't work. proj.files is itself failing, not on an individual file (so log never gets hit).
Adding logs to see if the project dir itself exists now - considering the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i don't see anyway to get logs from our unit tests. what have you used in the past @brunobowden ? they all seem piped away to report files we can't access on CI and neither you nor I have a Windows machine :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you access any logs from the machine?

I remember that Info or Debug logs were suppressed... but terrors and
Warnings came through. So abuse the Error logging for this.

On Thu, Oct 1, 2015, 2:13 AM Advay Mengle [email protected] wrote:

In
src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/TranslateTaskTest.groovy
#474 (comment)
:

     IncrementalTaskInputs incrementalTaskInputs = new IncrementalTaskInputs() {
         @Override
         boolean isIncremental() { return false }

         @Override
  •        void outOfDate(Action<? super InputFileDetails> outOfDateAction) {}
    
  •        void outOfDate(Action<? super InputFileDetails> outOfDateAction) {
    
  •            proj.files(Utils.isWindows() ? 'src\**\*.java' : 'src/**/*.java').each {
    
  •                final File file ->
    

actually i don't see anyway to get logs from our unit tests. what have you
used in the past @brunobowden https://github.com/brunobowden ? they all
seem piped away to report files we can't access on CI and neither you nor I
have a Windows machine :)


Reply to this email directly or view it on GitHub
https://github.com/j2objc-contrib/j2objc-gradle/pull/474/files#r40893186
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not that.

nothing you output during a unit test is ever displayed in the tes ttask

project.files(getTranslateSourcepaths()),
project.files(getGeneratedSourceDirs())
])
doTranslate(sourcepathDirs, srcGenTestDir, testTranslateArgs, testSrcFilesChanged)
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will translate the "Main" code twice? What about duplicating the "Main" code and only translating the "Test" code in the second step? This would reduce the translation but where does this break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. remember that sourcepath is NOT the list of things being translated (except when using --build-closure and the files in that directory are used transitively).

the list of files translated is testSrcFilesChanged in that call - which includes no main files

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, makes sense. Got it.

@brunobowden
Copy link
Contributor

I'm not sure what's going on with the Windows build but I believe my instinct is correct about the case. It would be helpful if you could add enough logging so that we can see what it's doing.

@@ -24,7 +24,11 @@ function runTest {
pushd $TEST_DIR
./gradlew wrapper
./gradlew clean
./gradlew build
# If we fail, try again with lots of logging.
./gradlew build || echo FAIL_LOG_FAIL_LOG_FAIL_LOG_FAIL_LOG && ./gradlew build --debug --stacktrace
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap with "****" as scanning the left hand side of a block of text, it's easier to spot this:

**** FAIL_LOG_FAIL_LOG_FAIL_LOG_FAIL_LOG **** 

Copy link
Contributor

Choose a reason for hiding this comment

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

Do that you if you can quote it... if it causes hassle for the script then drop the suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used underscores instead of starts for that reason

@advayDev1
Copy link
Contributor Author

@brunobowden - windows fixed. there were 2 bugs:
0. Misunderstanding of project.files(...) - it doesn't accept globs, just arrays of individually named files.

  1. MockProjectExec's handling of multiple arguments to project.files

nothing about windows!

@advayDev1
Copy link
Contributor Author

ptal @brunobowden - i think this is good to go

// files(ArrayList) possibly, so cast ArrayList to Object[]
return metaMethod?.invoke(delegate, [(Object[]) args.first()] as Object[])
} else {
return metaMethod?.invoke(delegate, [args] as Object[])
Copy link
Contributor

Choose a reason for hiding this comment

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

Does metaMethod?.invoke(delegate, [args] as Object[]) work for the args.size() == 1 case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't...
L96 is equivalent to invoke(delegate, [[args[0][0], args[0][1], args[0][2]]])
L98 is equivalent to invoke(delegate, [[args[0], args[1], args[2]]])

@brunobowden
Copy link
Contributor

LGTM

See if you can simplify the line within MockProjectExec. Either way, we're good to go.

advayDev1 added a commit that referenced this pull request Oct 1, 2015
Separate test translation and building from production library
@advayDev1 advayDev1 merged commit 30bb054 into j2objc-contrib:master Oct 1, 2015
@advayDev1 advayDev1 deleted the testSep branch October 2, 2015 08:44
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