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

serde: fix deserialize #837

Merged
merged 4 commits into from
Feb 17, 2023
Merged

serde: fix deserialize #837

merged 4 commits into from
Feb 17, 2023

Conversation

ejulio-ts
Copy link
Contributor

when rendering the following diagram with TALA, it errored when adding no collision.near: game loop.collision detected

direction: down
first input -> start game -> game loop

game loop: {
  direction: down
  input -> increase bird top velocity

  move bird -> move pipes -> render

  render -> no collision -> wait 16 milliseconds -> move bird
  render -> collision detected -> game over
  no collision.near: game loop.collision detected
}

The reason was that when deserializing the graph, game loop.Children had the full path game loop.collision detection instead of just collision detection. Then, TALA failed to find the near object and resulted in a panic.

Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

Could you move the test alex added to the monorepo here into e2etests?

@ejulio-ts
Copy link
Contributor Author

The e2etest? If so, I can add it, but I don't see a good reason for it. It's not testing anything new and we're not looking for its aesthetics. For example, here it wouldn't catch this bug because it was specific to TALA.

@ejulio-ts ejulio-ts requested a review from nhooyr February 17, 2023 18:55
@nhooyr
Copy link
Contributor

nhooyr commented Feb 17, 2023

The e2etest? If so, I can add it, but I don't see a good reason for it. It's not testing anything new and we're not looking for its aesthetics. For example, here it wouldn't catch this bug because it was specific to TALA.

Well it's not specific to TALA but that's the only part where we currently use the serialization code and the tests don't have an option to use external dagre yet so yea it's fine for now then.

Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

@ejulio-ts
Copy link
Contributor Author

Maybe, it makes sense to keep it in TALA so we detect any changes there ASAP

@ejulio-ts ejulio-ts merged commit a28dcd3 into terrastruct:master Feb 17, 2023
@ejulio-ts ejulio-ts deleted the fix-serde branch February 17, 2023 18:57
@ejulio-ts ejulio-ts changed the title serde: fiz deserialize serde: fix deserialize Feb 18, 2023
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.

2 participants