CocoonService uses relative domain for API on web #522
Conversation
CaseyHillers
changed the title
|
@jonahwilliams is there a way to configure the |
|
You can also run tests with |
| final String getStatusUrl = kIsWeb | ||
| ? '/api/public/get-status' | ||
| : '$_baseApiUrl/api/public/get-status'; | ||
|
|
There was a problem hiding this comment.
Consider encapsulate this pattern into a function, so that this pattern isn't scattered in 3 functions. For example,
// This function helps the Web app talks to a local Cocoon AppEngine instance when running locally, or talks to
// the production instance when deployed into production.
// The urlSuffix begins with a slash, e.g. "/api/public/get-status".
String getEndpoint(String urlSuffix) {
return kIsWeb ? urlSuffix : 'https://flutter-dashboard.appspot.com$urlSuffix;
}
There was a problem hiding this comment.
Thank you! Added.
|
Should I report differences in the platform tests?
For now, I can just have Cirrus run this platform test on this particular test. |
|
@jonahwilliams I added tests and added the chrome test for Cirrus. Currently only runs this group of tests as there are some failures. |
|
Can you file an issue for the lack of the network exception on the web? |
|
Of course! Do you know if there is a best practice for installing I noticed flutter/flutter just installs it in the Dockerfile https://github.com/flutter/flutter/blob/master/dev/ci/docker_linux/Dockerfile |
| /// before falling back to the production endpoint. | ||
| /// | ||
| /// This function helps the Web app talks to the Cocoon service relative to | ||
| /// where it's running. On mobile, default to use the production |
There was a problem hiding this comment.
Explain the magic: "On Web, a relative url automatically resolves to a local Cocoon instance when available, otherwise it resolvs to the production one. Therefore this function returns a relative url for Web.
Mobile apps don't support relative url."
There was a problem hiding this comment.
Okay. I expanded to explain more on the relative url, and why we do this.
|
|
||
| if (response.statusCode != HttpStatus.ok) { | ||
| print(response.body); | ||
| return CocoonResponse<List<CommitStatus>>() | ||
| ..error = | ||
| '$_baseApiUrl/public/get-status returned ${response.statusCode}'; | ||
| ..error = '/api/public/get-status returned ${response.statusCode}'; |
There was a problem hiding this comment.
Why this line doesn't call _apiEndpoint?
There was a problem hiding this comment.
It's just an error message, so I think just giving the endpoint is fine. I was trying to keep the error messages concise so they could be shown in the UI.
| }); | ||
| }); | ||
| } | ||
|
|
||
| class MockHttpClient extends Mock implements Client {} |
There was a problem hiding this comment.
Looks like a good use case to put the new tests into a group: https://flutter.dev/docs/cookbook/testing/unit/introduction#5-combine-multiple-tests-in-a-group
And a group can avoid the redundant setup code. Is that right? @jonahwilliams
There was a problem hiding this comment.
Right now the tests are grouped by the endpoint they relate to. Should we swap it so we group by the functionality under test?
|
There might be a better approach, but that's what I came up with in 5 minutes. I would sync with the rest of the infra team. |
|
Filed difference in test bug here: flutter/flutter#44370 |
|
@jonahwilliams is it okay to table running the tests on chrome on Cirrus? We can't add Google chrome via the cirrus yml, so that is going to be a bigger change that requires syncing with the infra team and more research. I can comment the line out on |
| }); | ||
| }); | ||
| } | ||
|
|
||
| class MockHttpClient extends Mock implements Client {} |
Closes flutter/flutter#43103
Allows for using production data on AppEngine versions that are not in production. Additionally, allows for use of local dart server.
Open questions
Is there a better way of building the URL? I think it is unnecessary to include the endpoint twice, but my only idea is to move it to a separate variable.