Skip to content

Conversation

stepansergeevitch
Copy link
Collaborator

Created a separate integration testing workflow to run a full integration test suite on account v2. Made some fixes and improvements (mainly to resource management) to make sure tests are passing

@stepansergeevitch stepansergeevitch self-assigned this May 22, 2024
@stepansergeevitch stepansergeevitch requested a review from a team as a code owner May 22, 2024 08:21
@stepansergeevitch stepansergeevitch changed the title tests: Integration testing account v2 test: Integration testing account v2 May 28, 2024
Copy link
Contributor

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

some minor comments

Comment on lines +68 to +69
"",
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have empty strings here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since fields names are accessed by index, we simply store empty strings to preserve order, yet we verify that this field is not accessed by an invalid account version. Converting this to a map would involve a lot of refactoring, yet we can do that if you have a strong opinion here. We should generally be able to simplify this list when we'll deprecate v1 engines fully

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, not feeling too strongly about it. Maybe let's just add a comment that this is left for compatibility.

@stepansergeevitch
Copy link
Collaborator Author

Copy link
Contributor

@ptiurin ptiurin left a comment

Choose a reason for hiding this comment

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

apart from that lgtm

Comment on lines +68 to +69
"",
"",
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, not feeling too strongly about it. Maybe let's just add a comment that this is left for compatibility.

Copy link

sonarqubecloud bot commented Jun 5, 2024

@stepansergeevitch stepansergeevitch merged commit 1773aac into main Jun 5, 2024
@stepansergeevitch stepansergeevitch deleted the integration-testing-account-v2 branch June 5, 2024 07:45
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