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

Add a federated test for TypeScript #646

Merged
merged 6 commits into from
Oct 21, 2021
Merged

Add a federated test for TypeScript #646

merged 6 commits into from
Oct 21, 2021

Conversation

hokeun
Copy link
Member

@hokeun hokeun commented Oct 20, 2021

Add a federated test for TypeScript after fixing issues with federated TypeScript code generation.

@hokeun hokeun changed the title Federated ts Add a federated test for TypeScdript Oct 20, 2021
@hokeun hokeun changed the title Add a federated test for TypeScdript Add a federated test for TypeScript Oct 20, 2021
@hokeun hokeun requested review from lhstrh and Soroosh129 October 20, 2021 10:23
Copy link
Contributor

@Soroosh129 Soroosh129 left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Nonetheless, I think we should hold off merging this until after #648 is fixed to avoid intermittent test failures.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Looks good!

@hokeun
Copy link
Member Author

hokeun commented Oct 20, 2021

Thanks, Soroush and Marten!

This looks good to me!

Nonetheless, I think we should hold off merging this until after #648 is fixed to avoid intermittent test failures.

I think #648 should not affect this one since the test only checks if this example successfully runs. That is, this should never fail due to #648, I think. I'll wait for your final comments before merging this. Let me know if you have any concerns!

@Soroosh129
Copy link
Contributor

Soroosh129 commented Oct 20, 2021

Oh, taking a second look at the test, shouldn't it keep track of the number of messages received by the PrintMessage reactor and fail if it's not 2?

I think at least a reaction(shutdown) should perhaps be in PrintMessage to confirm that at least one message is received (but I would argue that exactly two messages should be received for the test to pass).

@hokeun
Copy link
Member Author

hokeun commented Oct 20, 2021

Oh, taking a second look at the test, shouldn't it keep track of the number of messages received by the PrintMessage reactor and fail if it's not 2?

I think at least a reaction(shutdown) should perhaps be in PrintMessage to confirm that at least one message is received (but I would argue that exactly two messages should be received for the test to pass).

I agree with you that this test needs to be updated to do a better check. I just wanted to make sure we have at least one test that makes sure federated execution itself compiles and works for the TypeScript first. I added a TODO to update the test once the behavior issue (https://github.com/lf-lang/lingua-franca/issues/648) is resolved. But I'd like to have this test in place before I fix that issue. Would this make sense to you?

@Soroosh129
Copy link
Contributor

Yes it makes perfect sense! Thanks for adding the TODO.

@hokeun
Copy link
Member Author

hokeun commented Oct 21, 2021

Yes it makes perfect sense! Thanks for adding the TODO.

Thanks, Soroush!

@hokeun hokeun merged commit e07d76c into master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants