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

docs: edit copy for tick() in testing and api docs #35697

Closed

Conversation

kapunahelewong
Copy link
Contributor

Fixes #35696

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #35696

What is the new behavior?

Style/copy edits to recent PR #33838

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mary-poppins
Copy link

You can preview 111004d at https://pr35697-111004d.ngbuilds.io/.

aio/content/guide/testing.md Outdated Show resolved Hide resolved
aio/content/guide/testing.md Outdated Show resolved Hide resolved
aio/content/guide/testing.md Outdated Show resolved Hide resolved
aio/content/guide/testing.md Outdated Show resolved Hide resolved
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Feb 27, 2020
@ngbot
Copy link

ngbot bot commented Feb 27, 2020

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    failure status "ci/circleci: lint" is failing
    failure status "ci/circleci: test" is failing
    failure status "ci/circleci: test_aio_local" is failing
    failure status "ci/circleci: test_ivy_aot" is failing
    pending status "google3" is pending
    pending status "pullapprove" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Nice! I just noticed this yesterday and wanted to open a PR about this. (Glad you beat me to it 😇)

packages/core/testing/src/fake_async.ts Outdated Show resolved Hide resolved
* processNewMacroTasksSynchronously, whether to invoke the new macroTasks, by default is
* false, means the new macroTasks will be invoked
* @param millis, the number of milliseconds to advance the virtual timer
* @param tickOptions, the options of `tick()`, which has a flag called
Copy link
Member

Choose a reason for hiding this comment

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

Not sure exactly what is wrong (maybe the commas after the param names), but these docs do not seem to be included on the API docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Move the param descriptions up before @usageNotes, like this:

@param fn A function that takes two arguments. 
  * *millis* The number of milliseconds to advance the virtual timer.
  * *tickOptions* The options to pass to the `tick()` function.

Then put the rest of the advice about using the options into the @usageNotes (rephrasing as appropriate and responding to George's notes 😄 )

packages/core/testing/src/fake_async.ts Outdated Show resolved Hide resolved
@kapunahelewong kapunahelewong removed the action: merge The PR is ready for merge by the caretaker label Mar 2, 2020
@kapunahelewong kapunahelewong added this to In Progress in docs Mar 2, 2020
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Mar 2, 2020
@mary-poppins
Copy link

You can preview 7eaef73 at https://pr35697-7eaef73.ngbuilds.io/.

@aikithoughts aikithoughts requested review from petebacondarwin and removed request for IgorMinar, kara and alxhub May 24, 2021 21:26
@mary-poppins
Copy link

You can preview 6b36b4f at https://pr35697-6b36b4f.ngbuilds.io/.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Just one small suggestion. But otherwise nice tidy up.

Comment on lines +46 to +47
* Any arguments passed when calling this returned function will be passed through to the `fn`
* function in the parameters when it is called.
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +64 to +65
* @param millis The number of milliseconds to advance the virtual timer.
* @param tickOptions The options to pass to the `tick()` function.
Copy link
Member

Choose a reason for hiding this comment

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

👍

*
* @param maxTurns
* @returns The simulated time elapsed, in millis.
* @param maxTurns A maximum number of turns.
Copy link
Member

Choose a reason for hiding this comment

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

So digging through the code, I think that maxTurns is basically the maximum number of times the scheduler will attempt to clear its queue before it gives up with an error. It's a kind of Time To Live (TTL) number.

@mary-poppins
Copy link

You can preview c5b31ca at https://pr35697-c5b31ca.ngbuilds.io/.

@aikithoughts aikithoughts requested review from gkalpak and removed request for alxhub and gkalpak May 24, 2021 22:47
@aikithoughts aikithoughts dismissed gkalpak’s stale review May 24, 2021 22:47

Pete and others have already reviewed this. I think we're good! Thanks, George!

@aikithoughts aikithoughts added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker labels May 24, 2021
@ngbot
Copy link

ngbot bot commented May 24, 2021

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/circleci: test_win" is failing
    pending forbidden labels detected: action: cleanup
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@aikithoughts aikithoughts added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker labels May 24, 2021
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label May 25, 2021
@ngbot
Copy link

ngbot bot commented May 25, 2021

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@zarend zarend closed this in d7768c6 May 25, 2021
zarend pushed a commit that referenced this pull request May 25, 2021
umairhm pushed a commit to umairhm/angular that referenced this pull request May 28, 2021
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this pull request Jun 4, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes effort1: hours merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: patch This PR is targeted for the next patch release type: bug/fix
Projects
docs
In Progress
Development

Successfully merging this pull request may close these issues.

Edit copy for tick function