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

nimbus-fml: Make the output deterministic. #6283

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

PieroV
Copy link
Contributor

@PieroV PieroV commented Jun 25, 2024

Changed some HashMaps with BTreeMaps to ensure the order of the output is deterministic.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
    • It doesn't add new features, it changes a data type used here and there
    • Maybe it could be worth to include a test to check the output always follows a certain order, in case could you help me for that? 🙂
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@PieroV PieroV force-pushed the nimbus-fml-deterministic-output branch from 3e5d8b9 to a7d200d Compare June 25, 2024 15:25
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 22.66%. Comparing base (8fd08c6) to head (db52b2b).

Files Patch % Lines
...port/nimbus-fml/src/intermediate_representation.rs 0.00% 2 Missing ⚠️
components/support/nimbus-fml/src/parser.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6283   +/-   ##
=======================================
  Coverage   22.66%   22.66%           
=======================================
  Files         332      332           
  Lines       29804    29804           
=======================================
  Hits         6754     6754           
  Misses      23050    23050           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@linabutler
Copy link
Member

Thanks so much for doing this, @PieroV!

This looks great to me—though it looks like the test fixtures might need updating, from those Clippy and test failures 👀—but let's have Charlie and Beth give the official review.

jeddai
jeddai previously requested changes Jul 15, 2024
Copy link
Member

@jeddai jeddai left a comment

Choose a reason for hiding this comment

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

Ye, Lina is right, there's a few things you'll need to adjust but otherwise this looks great!

For clippy, run this from the base repo directory and it'll show you its issues: ./automation/tests.py rust-clippy

cargo test in the /components/support/nimbus-fml folder should show you the test failures.

All-in-all I think there shouldn't be too much to change to get this in a spot where it's good to merge!

@PieroV PieroV force-pushed the nimbus-fml-deterministic-output branch from a7d200d to d739b1a Compare July 16, 2024 13:26
@mergify mergify bot dismissed jeddai’s stale review July 16, 2024 13:26

The pull request has been modified, dismissing previous reviews.

@PieroV
Copy link
Contributor Author

PieroV commented Jul 16, 2024

I tried to run cargo teston the rebased branch, but I can't find test failures, at most some tests are ignored.

Should I pass additional arguments?

Changed some HashMaps with BTreeMaps to ensure the order of the output
is deterministic.
@PieroV PieroV force-pushed the nimbus-fml-deterministic-output branch from d739b1a to db52b2b Compare July 16, 2024 14:00
Copy link
Member

@jeddai jeddai left a comment

Choose a reason for hiding this comment

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

Looks great!! Thank you for your contribution ❤️

@jeddai jeddai added this pull request to the merge queue Jul 17, 2024
Merged via the queue into mozilla:main with commit d2a2e76 Jul 17, 2024
16 checks passed
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.

4 participants