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

Feature/build server #9

Merged
merged 113 commits into from
Nov 6, 2021
Merged

Feature/build server #9

merged 113 commits into from
Nov 6, 2021

Conversation

HenrikThoroe
Copy link
Collaborator

  • Exposes compiler via web socket API
  • Manages
    • many-clients one-project
    • many-clients many-projects
  • Resource management by assigning each project a custom isolate
  • Includes utilities for easier async event handling

@HenrikThoroe
Copy link
Collaborator Author

VHook

Don't get me wrong, it's a good tool. But for now it's lacking a few important functionalities and produces quite a bit overhead.
Would be nice if you could improve on it. If you don't have time I could do it as well.

Below I listed what I currently miss.

Overhead

// Would be nice to combine expect and awaitValue methods
await hook.expect(equals(*), timeout: Duration(years: 42));

// For now we have to use a two-liner for this
await hook.awaitValue(Duration(years: 42));
hook.expect(equals(*));

Functionality

  • Have to use bool? if I only want to distinguish between two states.
  • No replacement for Completer.completeError
  • If my value can be null I cannot await it
  • I'd like to await based on the value and not be limited to null checks. For example awaitValue(Duration(years: 42), (value) => condition)

@HenrikThoroe
Copy link
Collaborator Author

Protocol Messages

I don't think we should test if JsonSerializable is doing it's job. I think the only reasonable tests for protocol messages are tests of custom methods and tests if everything is available in ProtocolDelegate.

@Coronon
Copy link
Owner

Coronon commented Sep 8, 2021

ProtocolMessage

I would at least like some kind of check if a message is equal to itself after serialization and deserialization as this could discover errors made by us, the programmers.

@Coronon
Copy link
Owner

Coronon commented Sep 8, 2021

VHook

Those VHook features seem like a great idea. I’ll attempt to implement them this week. I could actually need them too :)

@HenrikThoroe HenrikThoroe mentioned this pull request Sep 8, 2021
@HenrikThoroe
Copy link
Collaborator Author

HenrikThoroe commented Sep 8, 2021

Answer to #9 (comment)

I do have one presumption on which my opinion is based:
We trust the json_serializable package.

That said we've got the following components in the message lifecycle:

  1. Conversion to JSON map
  2. Conversion to string which carries the type
  3. Conversion back to a map and getting the correct type without explicit type annotation
  4. Conversion back to an object

Step 1 and 2 don't have to be tested because the presumption. Step 2 and 3 need testing but in the scope of ProtocolDelegate which performs those steps. If we verify that ProtocolDelegate can do that on every possible object the only thing left to check is that our new message types are available in ProtocolDelegate.elements. All further tests would just test json_serializable.

@Coronon
Copy link
Owner

Coronon commented Sep 9, 2021

Answer to #9 (comment)

But testing json_serializable is not that bad of an idea. I don’t mean the actual serialization but rather the custom behaviour (key names, defaults etc.) set by us developers.

HenrikThoroe and others added 2 commits November 5, 2021 22:58
Co-authored-by: Rubin Raithel <33808743+Coronon@users.noreply.github.com>
@HenrikThoroe HenrikThoroe merged commit 33f851c into master Nov 6, 2021
@Coronon Coronon deleted the feature/build-server branch November 8, 2021 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants