search

apache / cordova-lib

Apache Cordova Tooling Library
https://cordova.apache.org/
Apache License 2.0
221 stars 243 forks source link

Bug(tests): Test#012 failure #906

Open breautek opened 1 year ago

breautek commented 1 year ago

Bug Report

Problem

1) pkgJson platform end-to-end with --save Test#012 : platform with
local path is added correctly with --save
  - Unhandled promise rejection: TypeError: message.split is not a
function
      at <Jasmine>
  - Error: Timeout - Async function did not complete within 150000ms
(set by jasmine.DEFAULT_TIMEOUT_INTERVAL)
      at <Jasmine>
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)

Information

This is a two-part issue.

message.split is an error inside of Jasmine because Jasmine expects either a string or a Error type object to be passed as a rejection message. But some of our errors are a CordovaError object. Jasmine doesn't know how to use CordovaError and crashes when trying to split it and results with a test failure by timeout (rather than reporting the actual error).

This however, is not the focus of this ticket. The actual underlying error is:

- Error: CordovaError: Error: npm: Command failed with exit code 127 Error output:
  npm ERR! code 127
  npm ERR! path /tmp/cordova-lib-pkgJson-fAGV7s/android
  npm ERR! command failed
  npm ERR! command sh -c -- cordova-js build > templates/project/assets/www/cordova.js
  npm ERR! sh: 1: cordova-js: not found

  npm ERR! A complete log of this run can be found in:
  npm ERR!     /home/norman/.npm/_logs/2023-01-01T20_04_09_361Z-debug-0.log
  npm ERR! code 127
  npm ERR! path /tmp/cordova-lib-pkgJson-fAGV7s/android
  npm ERR! command failed
  npm ERR! command sh -c -- cordova-js build > templates/project/assets/www/cordova.js
  npm ERR! sh: 1: cordova-js: not found
  npm ERR! A complete log of this run can be found in:
  npm ERR!     /home/norman/.npm/_logs/2023-01-01T20_04_09_361Z-debug-0.log
      at /development/cordova/coho/cordova-lib/integration-tests/pkgJson.spec.js:420:27
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

The test has a folder structure built that has 2 folders, android/, a cordova-android installation and project/, a cordova project for the purpose of the test.

There are two probems with android/ cordova-android installation. cordova-android has a prepare lifecycle NPM script, primarily intended to trigger just before NPM "packs" the package. However NPM will trigger prepare in some situations, including when installing from git or when doing local installs.

Problem 1

Because this is a local directory, NPM is symlinking android/ as cordova-android install, without installing it's dependencies, therefore cordova-js doesn't exists. If you enter android/ and run NPM install it will correct this issue, but cordova-js will error leading into Problem 2

Problem 2

Even if cordova-js was installed, the android/ structure is missing cordova-js-src directory and therefore cannot build the cordova.js file. The directory is missing because android/ gets populated via an npm install / copy from node_modules. cordova-js-src is in the GIT repository, but is .npmignore'ed, because it's not required since the cordova.js file will be built before the tarball is packed. This is also evident because the cordova.js file does already exists.

The Root of the Problem

The described problem are a side effect of the root problem, we aren't testing the right thing. Test#012 is testing a situation that looks something like:

User retrievies an NPM packed tarball file (e.g. cordova-android.tgz). User unpacks the tarball file. User tries to cordova platform add cordova-android/tarballDirectory

This is a weird scenario to test and is likely unintentional. If the user has a tarball file, they should simply add the tarball file (e.g: cordova platform add cordova-android.tgz). They shouldn't manually extract the tarball and add the extracted directory. That doesn't make sense.

Installing a platform from a directory is typically done if someone has a development version of a platform, most likely from SVC repository like Git. Therefore the android/ test artefact should reflect that kind of situation instead.

Command or Code

If the test cleanup code is commented out so that the temporary files remain in tact after attempting to run them, you can manually trigger the error by attempting to add the android/ directory as a platform, e.g: cordova platform add ../android (from within the project/ folder)

Environment, Platform, Device

Test Environment

Version information

cordova-lib master / v11.1.0 (prerelease)

Checklist

purplecabbage commented 1 year ago

Thanks for the thorough explanation! I was looking into this as I thought it was new as a result of my recent pr.

I am a 👍 to remove the test, is that your recommendation?

breautek commented 1 year ago

I am a +1 to remove the test, is that your recommendation?

No, I don't think the test should be removed, the act of testing adding local folders as a platform target should be something that is tested as that is a supported way of using Cordova... and I believe that is the intended test scenario in Test#012. However, it should be testing a valid directory as a platform target (e.g. not the contents of an NPM packed tarball but rather a git cloned directory, for example).

I'm not sure what is involved on making that happen, so we could disable the test with a comment pointing to this ticket for context until we do find a proper solution.

Or if we decide that an unpacked tarball folder should be a valid target, then we will probably have to fix cordova-android and potentially other platforms, but I'd rather call that an unsupported target (install the tarball itself instead).

erisu commented 1 year ago

@raphinesse what is your thoughts on this issue since them problems seems to be related to the new js build system.

raphinesse commented 1 year ago

I agree with @breautek that we should make sure we are testing the right thing. And if nobody has free cycles to do this, deactivating the test sounds reasonable.

Don't we have a minimal test platform fixture without dependencies in this repo already? I'm not sure, it's been some time.

Re the error issue: IIRC, CordovaError is an instance of Error, has a string message property and is nicely convertible to string (see the spec: https://github.com/apache/cordova-common/blob/b782a724f9fee6c483514ed9c23e4f20c8f9b8e6/spec/CordovaError/CordovaError.spec.js). Do you have a link to the jasmine code that throws the error?

breautek commented 1 year ago

Re the error issue: IIRC, CordovaError is an instance of Error, has a string message property and is nicely convertible to string (see the spec: https://github.com/apache/cordova-common/blob/b782a724f9fee6c483514ed9c23e4f20c8f9b8e6/spec/CordovaError/CordovaError.spec.js). Do you have a link to the jasmine code that throws the error?

No I don't and perhaps I'm wrong about that the cause here.. What I did was a .catch the promise to obtain the error and wrapped it in a new Error() and threw that instead, and jasmine seemed to reported the actual error instead of the message.split error. This is why I assumed that giving Jasmine a CordovaError was the problem, but if it's an instance of Error then I'd expect that it would work...

Perhaps we should create a new issue for this topic though.