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

Remove generated message populators from pkg/ttnpb #342

Closed
htdvisser opened this issue Mar 21, 2019 · 5 comments · Fixed by #4628
Closed

Remove generated message populators from pkg/ttnpb #342

htdvisser opened this issue Mar 21, 2019 · 5 comments · Fixed by #4628
Assignees
Labels
c/shared This is shared between components technical debt Not necessarily broken, but could be done better/cleaner
Milestone

Comments

@htdvisser
Copy link
Contributor

Summary:

The generated message populators in pkg/ttnpb cause more problems than they solve. In my experience they're only causing flaky tests because of failed validations and non-deterministic code coverage because of randomly taken branches.

What do you see now?

  • A mix of generated populators (that usually build garbage) and handwritten populators (that to a slightly better job)
  • Randomly failing (or randomly passing) Travis builds
  • Changes in code coverage without any code changes

What do you want to see instead?

The end goal is that our tests become deterministic, so ideally I'd want to see (handwritten or rule-aware) populators that have correct and deterministic output for a PRNG with the same seed.

How do you propose to implement this?

  • Investigate if we can get more deterministic results with a PRNG with a static seed.
  • Get rid of generated populators that build invalid structs
  • Investigate if we can generate populators with the validation rules as constraints

What can you do yourself and what do you need help with?

I think it's best if @rvolosatovs looked into this, because protoc and ttnpb is his domain. Let me know if you need any help.

@htdvisser htdvisser added bug Something isn't working c/shared This is shared between components labels Mar 21, 2019
@htdvisser htdvisser added this to the Next Up milestone Mar 21, 2019
@rvolosatovs
Copy link
Contributor

Populators are fully deterministic.
The issue is that in some tests we run those concurrently, which is where the non-determinism comes from.

@htdvisser htdvisser removed the bug Something isn't working label Apr 24, 2019
@johanstokking johanstokking added the technical debt Not necessarily broken, but could be done better/cleaner label Jun 20, 2019
@htdvisser htdvisser modified the milestones: Next Up, v3.12.0 Mar 2, 2021
@htdvisser htdvisser modified the milestones: v3.12.0, 2021 Q2 Mar 26, 2021
@htdvisser htdvisser added the needs/triage We still need to triage this label Jun 3, 2021
@nejraselimovic nejraselimovic removed the needs/triage We still need to triage this label Jun 7, 2021
@nejraselimovic
Copy link
Contributor

@htdvisser is this important enough to leave it scheduled for Q2?

@htdvisser
Copy link
Contributor Author

We're slowly starting the big refactoring of pkg/ttnpb. The end goal is to migrate away from gogo/protobuf completely, and removing the message populators (that are generated by gogo/protobuf) is one part of that.

I don't really enjoy doing these kind of chores, so I'd rather just get it over with than keep pushing it forward.

@htdvisser
Copy link
Contributor Author

htdvisser commented Jul 26, 2021

Pull request #4450 removed all unused populators, so now it's time to remove all usage of the remaining ones, and remove those too. From what I can see, we still use populators in these places:

@johanstokking
Copy link
Member

@KrishnaIyer for removing populators from pkg/gatewayserver/io/udp; just remove generatePushData() and instantiate uplink messages as needed.

@adriansmares the stuff in pkg/networkservers seems really easy to clean up; the populators are just there to have a valid state, not to do anything with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/shared This is shared between components technical debt Not necessarily broken, but could be done better/cleaner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants