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

Temporarily disable the VariantAnimationTimelineTest. #1946

Conversation

timothyfroehlich
Copy link
Member

@timothyfroehlich timothyfroehlich commented Dec 26, 2024

Based on PR #1943

Chain of upstream PRs as of 2024-12-26

It tries to access parts of the DesignDoc that aren't meant to be accessed, and fails to compile because Validation does not have the protobuf libraries as a dependency. We don't want to expose the Protobuf API, because this can lead to compilation problems when another library uses a different version of the Protobuf library. We'll need to change the test to resolve the compilation error without modifying it's Gradle dependencies.

Task to fix it is #1945

@timothyfroehlich timothyfroehlich self-assigned this Dec 26, 2024
@timothyfroehlich timothyfroehlich marked this pull request as ready for review December 26, 2024 18:27
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/temp-disable-variant-animation-test branch from 2c9e656 to bd572fa Compare December 26, 2024 18:33
@rylin8
Copy link
Collaborator

rylin8 commented Dec 26, 2024

What part of DesignDoc does it try to access? Please use multi-line comments /* */ so we don't see a line changed in the history of the file for every single line. It makes blame harder to use.

@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/delete-proto-utils branch from a44231a to c823e91 Compare December 26, 2024 20:14
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/temp-disable-variant-animation-test branch from bd572fa to bd9e855 Compare December 26, 2024 20:17
@timothyfroehlich
Copy link
Member Author

What part of DesignDoc does it try to access? Please use multi-line comments /* */ so we don't see a line changed in the history of the file for every single line. It makes blame harder to use.

Something related to line 82 of VariantAnimationTimelineTest:

                    val regexMatch = tNumRegex.find(context.from?.name ?: "")

Good call on using multi-line comments.

@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/temp-disable-variant-animation-test branch from bd9e855 to 8900fa6 Compare December 26, 2024 20:19
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/temp-disable-variant-animation-test branch from 8900fa6 to c6f1a0c Compare December 26, 2024 21:00
@rylin8
Copy link
Collaborator

rylin8 commented Dec 26, 2024

I'm fine fixing this test later, but I don't really get what the issue is with accessing part of the design doc. It just uses the context passed into the custom animation function to see what the source and destination node names are, and that's passed in from DesignCompose in the animation code.

@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/delete-proto-utils branch from c823e91 to 2467781 Compare December 26, 2024 21:46
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/temp-disable-variant-animation-test branch from c6f1a0c to 2467781 Compare December 26, 2024 21:47
timothyfroehlich added a commit that referenced this pull request Dec 26, 2024
<!-- start git-machete generated -->

# Based on PR #1947

## Chain of upstream PRs as of 2024-12-26

* PR #1886:
  `main` ← `feature/protoconv`

  * PR #1941:
    `feature/protoconv` ← `wb/froeht/protolize-dcf`

    * PR #1943:
      `wb/froeht/protolize-dcf` ← `wb/froeht/delete-proto-utils`

      * PR #1946:
`wb/froeht/delete-proto-utils` ←
`wb/froeht/temp-disable-variant-animation-test`

        * PR #1947:
`wb/froeht/temp-disable-variant-animation-test` ←
`wb/froeht/remove-serde-from-kotlin`

          * **PR #1948 (THIS ONE)**:
`wb/froeht/remove-serde-from-kotlin` ←
`wb/froeht/resolve-runtime-exceptions`

<!-- end git-machete generated -->

Protobuf maps are immutable and need to be modified by copying them.
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