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

chore(TIMOB-26695): only use debug logs in core-sdk development #10572

Merged
merged 3 commits into from Mar 25, 2019

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn commented Jan 7, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26695

Note: This does not require unit- or ui-tests, since it is an internal change only and is only applied to the Debug version of the target.

@build build added this to the 8.0.0 milestone Jan 7, 2019
@build build requested a review from a team January 7, 2019 13:40
@build
Copy link
Contributor

build commented Jan 7, 2019

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖 ❌ 4 tests have failed There are 4 tests failing and 449 skipped out of 2995 total tests.

Tests:

Classname Name Time Error
android.emulator.Titanium.UI.TabGroup #setActiveTab() 2.786 timeout of 2000ms exceeded
android.emulator.Titanium.UI.WebView ignoreSslError 1.898 done() invoked with non-Error: [object Object]
ios.Titanium.UI.WebView ignoreSslError 30.002 timeout of 30000ms exceeded
ios.Titanium.UI.WebView sslerror 30.001 timeout of 30000ms exceeded

Generated by 🚫 dangerJS against 30b30cd

@janvennemann
Copy link
Contributor

janvennemann commented Jan 8, 2019

+1 for the general idea of this. All the module loading debug is sometimes really cluttering the logs and hides the output that i'm actually interested in.

However, this would remove this debugging info completely for end users, right? It might be nice to still display this info just for the case that a file wasn't found. So a user knows where we looked for his required file and can check what's wrong with his require/import statement (typos or whatever).

@hansemannn
Copy link
Collaborator Author

I agree, but even for that case, a reference to the Node.js algorithm could be posted. Since the lookup is incremental, we would need to store the lookup until a successful or failed lookup and discard it if not necessary. I'd really align to Node.js here and as it seems, they only show a trace of the require statement itself.

@mukherjee2 mukherjee2 modified the milestones: 8.0.0, 8.0.1 Jan 16, 2019
@sgtcoolguy
Copy link
Contributor

We do report when a file is not found, but yeah we don't report the full listing of files we try. The output here is incidental - it's not really intended to be exposed and with my new fixes for require the listing of files/directories we try to load won't be displayed anyways. This only spits out when we try to load an encrypted asset. So, long story short: I think this should get merged in for 8.0.1.

@sgtcoolguy sgtcoolguy modified the milestones: 8.0.1, 8.1.0 Jan 23, 2019
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

This change is fine to me. Most of the original problem is actually solved by PR #10640 but this is debug level output that we shouldn't really be spitting out typically.

@sgtcoolguy
Copy link
Contributor

Mental note to self: needs to be cherry-picked to 8_0_X

@hansemannn
Copy link
Collaborator Author

Can this be merged in master? We'd love to use this in our app.

@ssekhri
Copy link

ssekhri commented Mar 4, 2019

FR Passed. The debug logs related to Node.js paths are not shown in the console.
Environment:
Studio: 5.1.2.201812191831
SDK: 8.1.0 local build
Mac OS: 10.14.3
Appc NPM: 4.2.13
Appc CLI: 7.0.10-master.17
Node: 8.12.0
NPM Ver: 6.4.1
Java Ver: 9.0.4
XCode: 10.1

@sgtcoolguy sgtcoolguy merged commit bf6c2c2 into tidev:master Mar 25, 2019
sgtcoolguy pushed a commit that referenced this pull request Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants