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

tests: remove XHRMock util #1291

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Conversation

peaBerberian
Copy link
Collaborator

This is a Work-In-Progress to remove the XHRMock utils from our tests, which monkey-patched the browser's XMLHttpRequest API to allow mocking requests, "locking" them, redirecting them and so on (kind of like sinon.js's nise which was a major inspiration but with added locking/unlocking capabilities).

The reasons for doing this are:

  • It only worked for XMLHttpRequest and not for the fetch browser API which we use to fetch low-latency segments for now (due to some API differences between the two) and plan to use for more things in the future.

    Adding support for fetch looks like huge work (we have to re-implement the most part of the API in JS - may be fun but there are funnier tasks which are also more useful :p)

  • We have RxPlayer API which allows an application to provide its own fetching logic (manifestLoader and segmentLoader) which covers almost all fetching cases (it does not cover DASH's XLink loading, low-latency segment fetching and future Content Steering Manifest fetching) and covers all fetching cases currently done by XHRMock anyway.

  • The now probable incoming future in-Worker feature of the RxPlayer cannot easily rely on monkey-patching native API it uses, at least in integration tests (well, in all honesty, we will have to think about how we'll be testing this).

  • It was complex, looked like an NIH-y fakeServer implementation.

    Removing it in favor or simpler solutions is just easier to understand.

This is not completely done yet.

@peaBerberian peaBerberian added work-in-progress This Pull Request or issue is not finished yet Priority: 3 (Low) This issue or PR has a low priority. labels Sep 26, 2023
@peaBerberian peaBerberian force-pushed the misc/remove-xhrmock-in-tests branch 2 times, most recently from 76ff342 to d0c38e4 Compare September 27, 2023 16:47
@peaBerberian peaBerberian force-pushed the misc/remove-xhrmock-in-tests branch 2 times, most recently from e46fcea to 54c1b65 Compare October 26, 2023 13:34
peaBerberian added a commit that referenced this pull request Oct 26, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
@peaBerberian peaBerberian force-pushed the misc/remove-xhrmock-in-tests branch 3 times, most recently from 37dba63 to 9d35029 Compare October 30, 2023 13:10
@peaBerberian peaBerberian changed the title [WIP] tests: begin removing XHRMock util tests: remove XHRMock util Oct 30, 2023
@peaBerberian peaBerberian modified the milestones: 4.0.0, 4.0.0-rc.1 Oct 30, 2023
This is a Work-In-Progress to remove the `XHRMock` utils from our tests,
which monkey-patched the browser's `XMLHttpRequest` API to allow mocking
requests, "locking" them, redirecting them and so on (kind of like
sinon.js's [nise](https://github.com/sinonjs/nise) which was a major
inspiration but with added locking/unlocking capabilities).

The reasons for doing this are:

  - It only worked for `XMLHttpRequest` and not for the `fetch` browser
    API which we use to fetch low-latency segments for now (due to some
    API differences between the two) and plan to use for more things in
    the future.

    Adding support for `fetch` looks like huge work (we have to
    re-implement the most part of the API in JS - may be fun but there
    are funnier tasks which are also more useful :p)

  - We have RxPlayer API which allows an application to provide its own
    fetching logic (`manifestLoader` and `segmentLoader`) which covers
    almost all fetching cases (it does not cover XLink charging,
    low-latency segment fetching and future Content Steering Manifest
    fetching) and covers all fetching cases currently done by `XHRMock`.

  - The now probable incoming future in-Worker feature of the RxPlayer
    cannot easily rely on monkey-patching its API, at least in integration
    tests (well, in all honesty, we will have to think about how we'll be
    testing this feature).

  - It was complex, looked like an NIH-y `fakeServer` implementation.

    Removing it in favor or simpler solutions is just easier to
    understand.

This is not completely done yet.
@peaBerberian peaBerberian merged commit 1556e53 into next-v4 Nov 6, 2023
3 checks passed
peaBerberian added a commit that referenced this pull request Nov 7, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Nov 13, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Nov 14, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Nov 14, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Nov 23, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 4, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 5, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 5, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 7, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 19, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 20, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Dec 22, 2023
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Jan 3, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Jan 3, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
@peaBerberian peaBerberian deleted the misc/remove-xhrmock-in-tests branch January 5, 2024 13:57
@peaBerberian peaBerberian restored the misc/remove-xhrmock-in-tests branch January 5, 2024 13:57
peaBerberian added a commit that referenced this pull request Jan 11, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Jan 11, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Jan 15, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
peaBerberian added a commit that referenced this pull request Jan 23, 2024
…contents

Thanks to a test rewrite linked to #1291, I noticed that the v4 broke
the possibility of relying on a `segmentLoader` when relying on smooth
streaming, due to forgetting to update some property name in a
refactoring of the transport code.

This commit/branch fixes just that.
@peaBerberian peaBerberian deleted the misc/remove-xhrmock-in-tests branch February 7, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 (Low) This issue or PR has a low priority. work-in-progress This Pull Request or issue is not finished yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant