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

Record and playback - POC for skip functionality in common recorder #4336

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Jul 17, 2019

Managing the list of test names would mean that we should frequently update them whenever a test name changes or a new test is added.
The plan is to avoid the skip list becoming stale in the following way.

Providing a .skip() method with the recorder so that it can be called from the it block to skip a test.

Let us take a moment to understand what happens with this change.

  • We no longer have to save/update the skip list
  • Test can be skipped by calling recorder.skip("browser"); or recorder.skip("node"); in the it block based on the environment we want the test to be skipped
  • In the record mode, the recording will be generated with the info right from when the recorder is invoked in beforeeach until the skip() method that is invoked in the it block.
    • We need to save(and commit) this recording for the playback mode since the recorder has no way to know that the test is being skipped until the skip() method is called.
      [ A small tradeoff for getting rid of skip list and still add the convenience of invoking the recorder in beforeeach ]

[WIP] Will rebase and merge this in the common recorder module which is in PR - #4281

/cc - @ramya-rao-a @XiaoningLiu @kinelski @sadasant

@HarshaNalluru HarshaNalluru self-assigned this Jul 17, 2019
@HarshaNalluru HarshaNalluru added the Client This issue points to a problem in the data-plane of the library. label Jul 17, 2019
@HarshaNalluru
Copy link
Member Author

Update Guildelines accordingly.

public skip(): boolean {
return skip.includes(this.filepath);
}
// public skip(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need commentted code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this will be deleted.
This current PR is just to show how the skip list can be avoided and hear some feedback for it.
Instead of doing the updates in recorder.ts files, I'm planning to update the new common recorder #4281.

@XiaoningLiu
Copy link
Member

recorder.skip("browser") requires us typing string "browser" everytime. Can we provide recorder.skipBrowser(), or require a union type like "browser" | "node" for the parameter?

@HarshaNalluru
Copy link
Member Author

Thanks, @XiaoningLiu, I like the recorder.skipBrowser() idea more. Will work on updating this.

@HarshaNalluru
Copy link
Member Author

#4898 is merged, Closing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants