titanium-sdk icon indicating copy to clipboard operation
titanium-sdk copied to clipboard

chore(test): add tests for Promise

Open drauggres opened this issue 5 years ago • 8 comments

JIRA: https://jira.appcelerator.org/browse/TIMOB-27483 Execution order for Promise is broken on iOS

MWE:

Promise.resolve()
  .then(() => console.log('A'));
console.log('B');

Correct output:

B
A

My guess is that jobs queue is drained after call to non-js-native methods (?). Tests with names like does not break execution order... are same as follows execution order... ones, except they have console.log calls.

UPD: in my tests it works correctly on ios 9 (9.3.5) and fail on ios 10+ (10.3.1, 12.1.1, 12.3.1, 13.1.2)

drauggres avatar Oct 03 '19 13:10 drauggres

Warnings
:warning: This PR has milestone set to 10.0.0, but the version defined in package.json is 10.2.0 Please either: - Update the milestone on the PR - Update the version in package.json - Hold the PR to be merged later after a release and version bump on this branch
Messages
:book: :fist: The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
:book:

:white_check_mark: All tests are passing Nice one! All 19694 tests are passing. (There are 1157 skipped tests not included in that total)

:book: :tada: Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. :thumbsup:
:book:

:floppy_disk: Here's the generated SDK zipfile.

Generated by :no_entry_sign: dangerJS against d492cac846bfa76add20e8484290f30a6fb8d6ee

build avatar Oct 03 '19 14:10 build

@drauggres Can you create / link a JIRA ticket for this? It looks quite critical, but to schedule it, I think Appc needs a ticket to track it. And does this also apply for async await blocks?

hansemannn avatar Oct 03 '19 14:10 hansemannn

Can you create / link a JIRA ticket for this?

Done

And does this also apply for async await blocks?

It does: async/await blocks are transpiled into Promises

drauggres avatar Oct 03 '19 15:10 drauggres

So my initial suspicion was that this was caused by the promise polyfill stuff. But even if I run on an iOS 13 sim and disable Promise polyfills (to use the native implementation) we see this issue.

  • I replaced the console.log() calls with Ti.API.info() calls, same issue.
  • Replace them with simple variable assignments, it works.
  • Replace with Ti.App.* property accesses, fails.
  • Replace with a for loop the just does some assignments/math - it works.
  • place a console.log() call in the top-level before appending to the result variable, but have none in the Promise then() implementation - fails.
  • Have none in top-level, but place one before modifying result in the Promise then function - works.

So it certainly looks like crossing the "bridge" has some impact on deferred things like Promises. If we hit a call to a Ti API in top-level/current context, it seems to first allow any scheduled Promises to fire first.

So let me try to illustrate what we should get versus what's happening:

let result = '';
Promise.resolve()
	.then(() => {
		result += '2';
	})
	.then(() => {
		result += '3';
	});

console.log('1');
result += '1';

In this example, result should eventually be '123', but is instead '231'. The call to console.log('1'); is basically letting the promise fire off. In my testing, a call to any of the Ti namespace methods does the same. So this is a pretty low-level issue in terms of how we're hooking to the JS engine.

sgtcoolguy avatar Oct 21 '19 17:10 sgtcoolguy

I still can reproduce this issue on master (so here is also a question about the testing environment).

I think this is really critical issue, because it is breaking code execution order. (Same as if 2+2*2 were returning 8 on iOS and 6 on Android)

drauggres avatar Jan 21 '20 10:01 drauggres

@drauggres Do you have a suggestion to move forward here? Happy to merge the new tests, but if I followed correctly, they focus on the example that iOS behaves differently, not on the fix so far. I can also try to take a look here, but I'm unsure that will be easy to fix.

hansemannn avatar Mar 21 '22 09:03 hansemannn

Initially, these tests failed on iOS. I don't see testing in the github actions now, but the previous build (d492cac commit) was done with jenkins and should have been tested. It is maked as successful, i.e. the problem was fixed (I don't know how or when).

gh

drauggres avatar Mar 21 '22 10:03 drauggres

Thats odd. @sgtcoolguy Do you know by any chance if there was a change here last year fixing that issue?

EDIT: Unfortunately not fixed. The following still returns A B:

Promise.resolve()
  .then(() => console.log('A'));
console.log('B');

hansemannn avatar Mar 21 '22 10:03 hansemannn