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

✅ [RUMF-815] import RRWeb integration tests #738

Merged
merged 5 commits into from
Feb 23, 2021

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Feb 18, 2021

Motivation

Import RRWeb integration tests as E2E tests. Since we are running those tests in various browsers and snapshots are heavily dependent of the browse, we cannot really use snapshot testing like RRWeb does.

Changes

  • can record form interactions
  • can record childList mutations
  • can record character data muatations
  • can record attribute mutation
  • can record node mutations
    skipped, too big to test, already covered by other tests
  • can freeze mutations
    skipped, feature have been removed
  • should not record input events on ignored elements
    was already implemented
  • should not record input values if maskAllInputs is enabled
    skipped, feature not used
  • can use maskInputOptions to configure which type of inputs should be masked
    skipped, feature not used
  • should not record blocked elements and its child nodes
  • should record DOM node movement 1
  • should record DOM node movement 2
  • should record dynamic CSS changes
  • should record canvas mutations
    skipped, feature not used
  • will serialize node before record
  • will defer missing next node mutation
    skipped, no relation with RRWeb
  • can record log mutation
    skipped, feature not implemented
  • should nest record iframe
    skipped, feature not implemented

Testing

E2E


I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner February 18, 2021 17:34
test/e2e/scenario/recorder.scenario.ts Outdated Show resolved Hide resolved
Comment on lines 123 to 147
await flushEvents()

expect(events.sessionReplay.length).toBe(1)

const segment = events.sessionReplay[0].segment.data
const fullSnapshot = findFullSnapshot(segment)!
const mutations = findAllIncrementalSnapshots(segment, IncrementalSource.Mutation) as Array<{
data: MutationData
}>

expect(mutations.length).toBe(1)

expect(mutations[0].data.adds).toEqual([
{
nextId: null,
parentId: findElementWithTagName(fullSnapshot, 'p')!.id,
node: jasmine.objectContaining({ tagName: 'span' }),
},
])
expect(mutations[0].data.removes).toEqual([
{
parentId: findElementWithTagName(fullSnapshot, 'body')!.id,
id: findElementWithTagName(fullSnapshot, 'ul')!.id,
},
])
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, there are a lot of similarities across these tests, we could probably make something to reduce the boilerplate and make them more expressive.

what would you think of having a test API like:

flushSessionReplayEvents(events)  // page object-like API to have acces to events, segment, full snapshot
  .singleRecord() // select record to check and do something similar for filterRecordsByIdAttribute usages
    .expectAdded ({  tag: 'span', parentTag: 'p' }) 
    .expectRemoved({ tag: 'ul', parentTag: 'body' })
    .expectNotAdded({ tag: 'li' })
    .expectNotRemoved({ tag: 'li' })

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try something

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so I did something to remove the boilerplate. Since I didn't write the original tests I wanted to keep some kind of exhaustive snapshot, so I did not use a expectAdded/expectNotAdded API, and instead add a helper to verify the whole mutations. Because nodes are referenced in the initial full snapshot or the previous mutations, it makes things a bit complex. Let me know what you think about it!

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/import-rrweb-integration-tests branch 2 times, most recently from 8f667d4 to 1ffb4f3 Compare February 22, 2021 11:17
@bcaudan
Copy link
Contributor

bcaudan commented Feb 23, 2021

LGTM except flaky tests

@codecov-io
Copy link

Codecov Report

Merging #738 (908ba4f) into master (7480da5) will decrease coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
- Coverage   82.54%   82.41%   -0.14%     
==========================================
  Files          74       74              
  Lines        3798     3798              
  Branches      903      903              
==========================================
- Hits         3135     3130       -5     
- Misses        663      668       +5     
Impacted Files Coverage Δ
packages/rum-core/src/transport/batch.ts 65.71% <0.00%> (-11.43%) ⬇️
...ckages/core/src/domain/automaticErrorCollection.ts 98.41% <0.00%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7480da5...908ba4f. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants