Implements a Context that talks to the store mock server #293

CarlosNihelton · 2023-09-22T05:36:53Z

This PR adds a replacement for the thin MS Store API wrapper by a not-so-thin client of the store mock server we presented in #268.
For ease of implementation I opted for writing a Windows specific version of the mock client. You'll see that it's indeed not that hard to talk to REST API's using WinRT. Since we adopted a very consistent calling convention in our REST API, a single call() function models very well talking to any of the exposed endpoints, so that the WinMockContext methods just wrap around it passing the appropriate arguments. The actual HTTP call could be more elaborated, using streamed data to prevent against large responses, but I don't believe this is necessary, because we control the server and we know the responses are rather small.

Since this client code is not trivial, I added a few test cases for it, but I didn't plug it in CI right away because running it is a bit cumbersome:

We need to acquire a port to run the store mock server
Export the environment variable UP4W_MS_STORE_MOCK_ENDPOINT with the address the mock server will run (localhost:port)
Run the store mock server passing that port and the testcase.yaml configuration file.
Export the environment variable UP4W_TEST_WITH_MS_STORE_MOCK so the replacement context takes the stage instead of the production one.
Configure and build with CMake
Run CTest.

Not that complicated, but requires Go to build the store mock server and a few lines of powershell scripting to do the port thingy. So I found that this is not the appropriate moment for pluging this on in the CI. Also, the mock client is for supporting testing higher level components, and this is testing it, not the targets.

Here's a gist of how I run on my computer:

cd ./storeapi/test
$listener = new-object System.Net.Sockets.TcpListener("127.0.0.1",0)
$listener.Start()
$env:UP4W_TEST_WITH_MS_STORE_MOCK=1
cmake -S . -B .\out\build\
cmake --build .\out\build
$env:UP4W_MS_STORE_MOCK_ENDPOINT="$($listener.LocalEndpoint.Address):$($listener.LocalEndopint.Port)";
$listener.Stop()

go run ..\..\mocks\storeserver\storeserver\main.go -a $env:UP4W_MS_STORE_MOCK_ENDPOINT .\storeapi\test\testcase.yaml  # On another terminal

ctest --test-dir .\out\dir --output-on-failure

Fixes Udeng-1258

Handles the inner details of calling HTTP Returns a JsonObject on success winrt::hresult_error exceptions will bubble up on failure, Including JSON parsing error.

To avoid code duplication The exact same implementation is needed in the mock client.

That requires having a constructor exposing all fields.

Maybe we grow it when we start using the mock in tests

in the CMakeLists.txt Similar to what was done with Flutter Detects the env var UP4W_TEST_WITH_MS_STORE_MOCK Defines the services test wth mocks Including a mock server config

CarlosNihelton · 2023-09-22T05:54:23Z

storeapi/base/impl/WinMockContext.cpp

+  std::chrono::system_clock::time_point tp{};
+  std::stringstream ss{winrt::to_string(obj.GetNamedString(L"ExpirationDate"))};
+  ss >> std::chrono::parse("%FT%T%Tz", tp);


This little gem is only available for GCC on trunk :(

https://godbolt.org/z/ocPvjMeaT

EduardGomezEscandell

This whole mock storeAPI is really cool 😄

Just a few minor comments.

storeapi/base/impl/WinMockContext.cpp

storeapi/base/impl/WinMockContext.hpp

EduardGomezEscandell · 2023-09-22T07:38:03Z

storeapi/base/impl/WinMockContext.cpp

@@ -84,6 +84,23 @@ std::vector<std::string> WinMockContext::AllLocallyAuthenticatedUserHashes() {
  return result;
 }

+std::string WinMockContext::GenerateUserJwt(std::string token,
+                                            std::string userId) const {
+  assert(!token.empty() && "Azure AD token is required");


I know this simply mirrors the store context, but I'm thinking that, in both contexts, this assertion should also be run on Release (or remove it and let the server fail, doesn't really matter).

If we leave it as-is, we have diferent failure modes in Release and Debug, so we won't be testing the right thing. Maybe we can create a card and deal with these in a future PR.

The idea of an assertion is to educate the programmers to ensure those checks won't be needed in release.
If we are unsure about whether we are safe to trust that those checks aren't needed in production, then we should revisit the usage of assertions and maybe convert into wide contracts with exceptions being thrown for invalid arguments.
I personally like assertions because they clearly state: "you'll called me outside of our contract, that's a bug, take your crash". Once we crash enough in debug mode, those checks shouldn't be necessary anymore. Exceptions are more fuzzy. What is an exceptional situation? As always, it depends...

I like having assertions mostly where an error is critical: to avoid segfaults or undefined behaviour. In this case, since this would error out anyways (just server-side), I'm not sure it is worth having a release/debug split. Particularly because we test error paths quite intensely, so we'd be testing a ficticious path here.

Since this is not performance-critital, it seems that it would not hurt to always return error, or to always let the server fail. Either way, this is not critical to this PR.

The JsonObject forward declaration seems good enough. No actual leakage of WinRT types to be consistent with the production API.

EduardGomezEscandell

Cool, cool, cool.

CarlosNihelton added 10 commits September 22, 2023 01:05

Implements the WinMockContext header

189e3ea

Implements the call() HTTP function wrapper

b8b92b0

Handles the inner details of calling HTTP Returns a JsonObject on success winrt::hresult_error exceptions will bubble up on failure, Including JSON parsing error.

Moves to_hstrings() to a new header

893dc4d

To avoid code duplication The exact same implementation is needed in the mock client.

Adds a Product factory fromJson

de69f7a

That requires having a constructor exposing all fields.

Implements GetProducts

752450e

Implements AllLocallyAuthenticatedUserHashes

09605ce

Implements GenerateUserJwt.

13bd038

InitDialogs is NOOP for now

7a78f2d

Maybe we grow it when we start using the mock in tests

Implements purchase

3ccb3b4

Support for testing the services with the mock

9d029e4

in the CMakeLists.txt Similar to what was done with Flutter Detects the env var UP4W_TEST_WITH_MS_STORE_MOCK Defines the services test wth mocks Including a mock server config

CarlosNihelton marked this pull request as ready for review September 22, 2023 05:46

CarlosNihelton requested a review from EduardGomezEscandell September 22, 2023 05:46

CarlosNihelton commented Sep 22, 2023

View reviewed changes

EduardGomezEscandell suggested changes Sep 22, 2023

View reviewed changes

Turns fromJson into a Product constructor

1615761

The JsonObject forward declaration seems good enough. No actual leakage of WinRT types to be consistent with the production API.

CarlosNihelton requested a review from EduardGomezEscandell September 22, 2023 13:48

EduardGomezEscandell approved these changes Sep 22, 2023

View reviewed changes

CarlosNihelton merged commit eda4994 into main Sep 22, 2023
30 checks passed

CarlosNihelton deleted the context-mock-udeng-1258 branch September 22, 2023 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements a Context that talks to the store mock server #293

Implements a Context that talks to the store mock server #293

CarlosNihelton commented Sep 22, 2023 •

edited

Loading

CarlosNihelton Sep 22, 2023

EduardGomezEscandell left a comment

EduardGomezEscandell Sep 22, 2023

CarlosNihelton Sep 22, 2023

EduardGomezEscandell Sep 22, 2023 •

edited

Loading

EduardGomezEscandell left a comment

Implements a Context that talks to the store mock server #293

Implements a Context that talks to the store mock server #293

Conversation

CarlosNihelton commented Sep 22, 2023 • edited Loading

CarlosNihelton Sep 22, 2023

Choose a reason for hiding this comment

EduardGomezEscandell left a comment

Choose a reason for hiding this comment

EduardGomezEscandell Sep 22, 2023

Choose a reason for hiding this comment

CarlosNihelton Sep 22, 2023

Choose a reason for hiding this comment

EduardGomezEscandell Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

EduardGomezEscandell left a comment

Choose a reason for hiding this comment

CarlosNihelton commented Sep 22, 2023 •

edited

Loading

EduardGomezEscandell Sep 22, 2023 •

edited

Loading