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

test(docs-infra): draft to ensure that everything is setup properly t… #34190

Open
wants to merge 2 commits into
base: master
from

Conversation

@sonukapoor
Copy link
Contributor

sonukapoor commented Dec 2, 2019

…o run tests

against the Rx-JS examples

Addresses #28017

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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@sonukapoor sonukapoor requested review from angular/docs-infra as code owners Dec 2, 2019
@googlebot googlebot added the cla: yes label Dec 2, 2019
@sonukapoor sonukapoor force-pushed the sonukapoor:unit-tests-for-stand-alone-code branch from 48592a0 to b1fcd71 Dec 2, 2019
@sonukapoor

This comment has been minimized.

Copy link
Contributor Author

sonukapoor commented Dec 2, 2019

@gkalpak Please review. The assumption is that the step to run the stand-alone tests runs after the e2e tests have run. This will ensure that the js files have been created. Let me know what you think.

@sonukapoor sonukapoor force-pushed the sonukapoor:unit-tests-for-stand-alone-code branch from b1fcd71 to dff4783 Dec 2, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 2, 2019
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 3, 2019

This lgtm for now. We will re-work it the infrastructure, before the merging the PR, but this is good enough for now so that you can keep working on the actual tests. Thx for sorting it out 👍

@sonukapoor sonukapoor force-pushed the sonukapoor:unit-tests-for-stand-alone-code branch 2 times, most recently from acf8db2 to 2e3651b Dec 6, 2019
…o run tests

against the Rx-JS examples

Addresses #28017
@sonukapoor sonukapoor force-pushed the sonukapoor:unit-tests-for-stand-alone-code branch from 2e3651b to 747e9d0 Dec 9, 2019
@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Dec 9, 2019

As discussed "offline", here are my thoughts (and suggestions for next steps):

  • A proper solution might involve setting up a different project type for Observable-related projects (e.g. not involving Angular dependencies and providing dependencies needed for observables only). This would be a little expensive in terms of both initial implementation and maintenance, though.

  • I realized that the observables examples are not offered for download/StackBlitz, so the users only really see the code snippets (not the whole files). This gives us some flexibility on putting stuff around the docregions that will help us with testing, but without confusing the users (which will never see that extra stuff).

  • I thought about using Karma for the tests. This would give us access to a real browser. I then realized that that would only solve part of the problem. We would need to add appropriate HTML files and that would take more time/maintenance/complexity and would most likely still require infrastructure changes. Not ideal.

  • Finally, I noticed another issue: The code in the files executes eagerly (i.e. as soon as the file is loaded), which means that we would have a hard time testing stuff in the test files, because it would have happened before even loading the test files.


So, given the above, I think a good solution ("good" in the sense that it is fairly simple, will not take a lot of time to implement infrastructure for and will allow us to focus on tests (which is the primary focus for now)) would be the following:

  • We could wrap each docregion (i.e. code snippet) in a function and export that function. This way, the code snippets that the user sees wouldn't change (because they would only see the function body), but the code would not be executed as soon as the file is loaded during tests.

  • In tests, we could call the exported functions to run the code. This gives us the freedom to:
    a. Set up spies and mocks before executing the function.
    b. Pass mocks for globals as arguments to the function. For example, we could pass a mock document to the function etc.

As an example, here is what a code snippet could look like before my suggested change:

// #docregion foo
const elem = document.querySelector('foo');
const subscription = fromEvent(elem, 'click').subscribe(() => console.log('*click*'));

setTimeout(() => subscription.unsubscribe(), 10000);
// #enddocregion foo

And here is how it would be transformed with my suggestion:

// By wrapping a function around the code, we make it "lazy"
// (i.e. it won't be executed immediately).
//
// We can pass in "globals" (in TS we can give them proper types).
// Here for example, we can pass in a mock `console` to capture the logs.
// (We could even pass a mock `setTimeout()`, but that is unnecessary,
// since we can mock that with `jasmine.clock().install()` etc.)
export function docregionFoo(console, document) {

  // The code-snippet that the user will see remains unchanged.
  // (The indentation might change, but that is fine.)
  // #docregion foo
  const elem = document.querySelector('foo');
  const subscription = fromEvent(elem, 'click').subscribe(() => console.log('*click*'));

  setTimeout(() => subscription.unsubscribe(), 10000);
  // #enddocregion foo

  // Here, return the `subscription`, if we need to run assertions against it.
  return subscription;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.