Skip to content

Conversation

@wastadtlander
Copy link

@wastadtlander wastadtlander commented Jul 6, 2022

Implemented the following:

  • genrules for executing binaries and generating output files.
  • cc_test and compiler_main_test for comparing output of binaries to expected output.

This change is Reviewable

@wastadtlander wastadtlander requested review from EvgSkv, tcsnfkx and wliue July 6, 2022 20:52
Copy link
Contributor

@tcsnfkx tcsnfkx left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @EvgSkv, @wastadtlander, and @wliue)


src/test/cc/wfa/virtual_people/training/model_compiler/compiler_main_test.cc line 30 at r1 (raw file):

namespace {

void ReadTextProtoFile(std::string path, google::protobuf::Message& message) {

Can we use absl::string_view?

Code quote:

std::string

src/test/cc/wfa/virtual_people/training/model_compiler/compiler_main_test.cc line 39 at r1 (raw file):

}

bool ProtoEquals(std::string expectedPath, std::string outputPath) {

Same as above.


src/test/cc/wfa/virtual_people/training/model_compiler/compiler_main_test.cc line 39 at r1 (raw file):

}

bool ProtoEquals(std::string expectedPath, std::string outputPath) {

Could you please change to "expected_path"?

Code quote:

expectedPath

src/test/cc/wfa/virtual_people/training/model_compiler/compiler_main_test.cc line 39 at r1 (raw file):

}

bool ProtoEquals(std::string expectedPath, std::string outputPath) {

Same as above.

Code quote:

outputPath

Copy link
Contributor

@tcsnfkx tcsnfkx left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EvgSkv, @wastadtlander, and @wliue)

Copy link
Contributor

@tcsnfkx tcsnfkx left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EvgSkv, @wastadtlander, and @wliue)

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.

3 participants