Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants