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

Replace preq with api-testing REST client #1281

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kevinbazira
Copy link
Member

Bug: T265846

I have been able to replace preq with the api-testing REST client within 11 out of 13 tests.

The two tests that are failing when I switch them to client.post are;

  1. The html2wt with scrub_wikitext test fails with error 400 whenever the scrub_wikitext: 1 parameter is added.

  2. The supports stashing content test fails at the second client.post.

Could there be something I am missing that is causing these two tests to fail?

@@ -15,123 +16,108 @@ describe('transform api', function() {
this.timeout(20000);
let contentTypes;
const server = new Server();
const client = new REST('');
Copy link
Contributor

@Pchelolo Pchelolo Nov 3, 2020

Choose a reason for hiding this comment

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

Nothing to do here in this pull request, but it would've been sweet if we could inject the config into the clients @clarakosi

Copy link
Member

Choose a reason for hiding this comment

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

So the current workaround for the JSON file would be to declare an environment variable for the base URI. Probably easiest to do it in the test command in package.json but that would complicate things when trying to run a single file. Will look into why we did it this way next week

@Pchelolo
Copy link
Contributor

Pchelolo commented Nov 3, 2020

Could there be something I am missing that is causing these two tests to fail?

Can you add the changes you made for these tests into the PR too? It's hard to debug not knowing what you've done.

@Pchelolo
Copy link
Contributor

Pchelolo commented Nov 3, 2020

The html2wt with scrub_wikitext test fails with error 400 whenever the scrub_wikitext: 1 parameter is added.

I've tried to change this one locally and it works for me.

The supports stashing content test fails at the second client.post.

So, this one requires you to set a if-match header and I haven't found a way to do it in api-testing. @clarakosi writes in the api-testing library under request jsdoc:

The request has not been sent when this method returns, and can still be modified like a superagent request.

So I've attempted to just use .set there, but to no success - since the 'api-testing.request' method is async, it returns a promise, and that JSDoc is incorrect. Thus, the api-testing library needs fixing - we need a way to set arbitrary headers to requests, at least in REST. You can work with @clarakosi on that.

@kevinbazira
Copy link
Member Author

Can you add the changes you made for these tests into the PR too? It's hard to debug not knowing what you've done.

The two tests that are failing have been added into the PR.

Hope we can find a way to make them work with the api-testing REST client.

Copy link
Member

@clarakosi clarakosi left a comment

Choose a reason for hiding this comment

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

I've updated the api-testing package to address some of the issues you were having. You can find the new changes in api-testing 1.4.0

test/features/parsoid/transform.js Show resolved Hide resolved
test/features/parsoid/transform.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/features/parsoid/transform.js Outdated Show resolved Hide resolved
@kevinbazira
Copy link
Member Author

@clarakosi and @Pchelolo, all instances of preq have been replaced with the api-testing REST client in transform.js and all 13 tests are passing. Thanks to both of you!

Copy link
Member

@clarakosi clarakosi left a comment

Choose a reason for hiding this comment

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

Well done! 🎉 One more nit below

.api-testing.config.json Outdated Show resolved Hide resolved
@kevinbazira kevinbazira force-pushed the replace_preq_with_api-testing_REST_client branch from f32444a to 0d28434 Compare November 10, 2020 17:17
package.json Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants