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

Convert done callbacks to async/await in test/unit/api_spec.js #13253

Merged

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Apr 16, 2021

This is one of the three remaining files, and since it's quite big it's done separately from the other ones for easier review. It's recommended to use the ?w=1 flag to make reviewing much easier because of all indentation changes.

@timvandermeij
Copy link
Contributor Author

/botio unittest

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

That's a lot of code, but hopefully I've not missed anything :-)

This looks mostly good, but I think that there's some additional clean-up possible if you're touching most of the file anyway.
I'd suggest re-factoring the promise usage a bit more while you're at it, please see the inline comment which shows one such example.

Also, can you please fix the mistake I made in https://github.com/mozilla/pdf.js/blob/master/test/unit/api_spec.js#L2116?

test/unit/api_spec.js Outdated Show resolved Hide resolved
test/unit/api_spec.js Outdated Show resolved Hide resolved
test/unit/api_spec.js Outdated Show resolved Hide resolved
@timvandermeij
Copy link
Contributor Author

timvandermeij commented Apr 17, 2021

The review comments have been addressed. Given the size of the file, it might be easiest to review the interdiff and ignoring all changes not in test/unit/api_spec.js (from the rebase onto master); please see https://github.com/mozilla/pdf.js/compare/9f8272faf75bca4d2c6d88ed6c8cfaf85fe24146..130c2547ec8ecbdd85988d6720eee89441d362cd#diff-bc84ed767f5905bdf8c544c715c08e9b751e7bb1db16c8182d9de515b56bc4b6.

@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@timvandermeij
Copy link
Contributor Author

/botio unittest

@timvandermeij timvandermeij force-pushed the unit-test-async-await-pt3 branch 2 times, most recently from 1d9bd7d to 77d8161 Compare April 17, 2021 14:00
@timvandermeij
Copy link
Contributor Author

I have also fixed the mistake you mentioned now.

/botio unittest

@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

That's a lot of boilerplate being removed here :-)

r=me, with one final nit; thank you for doing this clean-up!

test/unit/api_spec.js Outdated Show resolved Hide resolved
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@mozilla mozilla deleted a comment from pdfjsbot Apr 17, 2021
@timvandermeij
Copy link
Contributor Author

/botio unittest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/b0589c0902b763f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://3.101.106.178:8877/1e850d4f92066cc/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/b0589c0902b763f/output.txt

Total script time: 3.83 mins

  • Unit Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/1e850d4f92066cc/output.txt

Total script time: 5.60 mins

  • Unit Tests: Passed

@timvandermeij timvandermeij merged commit c86e70b into mozilla:master Apr 17, 2021
@timvandermeij timvandermeij deleted the unit-test-async-await-pt3 branch April 17, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants