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

Run generator tests on Travis #2111

Conversation

rachel-fenichel
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Generator tests have not previously been scripted.

Proposed Changes

For each language:

  • run generator
  • save generated code to a tmp file
  • compare the generated file against a golden file

I also updated some of the test names to get rid of name conflicts in functions between text and list test, which were causing some tests to not be run when loading all of the tests with appendDomToWorkspace.

I verified this fix by checking that the same number of tests are run when loading test suites individually and when loading them all together.

Reason for Changes

Generator tests are frustrating.

Test Coverage

Tested by running the run_all_tests script locally. I also ran the golden code for each language and verified that all tests pass. For PHP I removed the known broken generated code (see #1201) and verified that the rest of the generated code works.

loadSelected();
})
})
.pause(10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are 10 second pauses the only way to make sure this works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldn't be. I'll look into it.

local tmp_filename="${TMP_DIR}generated.$suffix"
if [ ! -f $tmp_filename ]; then
echo "File $tmp_filename not found!"
FAILURE_COUNT=$((FAILURE_COUNT+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return if the file doesn't exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to do the next steps if the file is found, and otherwise simply report the error.

local golden_filename="${GOLDEN_DIR}generated.$suffix"
if [ ! -f $golden_filename ]; then
echo "File $golden_filename not found!"
FAILURE_COUNT=$((FAILURE_COUNT+1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto.

Copy link
Collaborator Author

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Travis is failing on OSX because it can't find the firefox driver. I'm investigating.

local tmp_filename="${TMP_DIR}generated.$suffix"
if [ ! -f $tmp_filename ]; then
echo "File $tmp_filename not found!"
FAILURE_COUNT=$((FAILURE_COUNT+1))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to do the next steps if the file is found, and otherwise simply report the error.

local golden_filename="${GOLDEN_DIR}generated.$suffix"
if [ ! -f $golden_filename ]; then
echo "File $golden_filename not found!"
FAILURE_COUNT=$((FAILURE_COUNT+1))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ditto.

loadSelected();
})
})
.pause(10000)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldn't be. I'll look into it.

Copy link
Contributor

@RoboErikG RoboErikG left a comment

Choose a reason for hiding this comment

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

LGTM once you get the mac build working.

@rachel-fenichel rachel-fenichel merged commit d677023 into google:develop Nov 14, 2018
@rachel-fenichel
Copy link
Collaborator Author

Followup issues: #2114, #2115, #2116, #2117, #2118.

@rachel-fenichel rachel-fenichel deleted the feature/run_gen_tests_in_browser branch May 1, 2019 17:44
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.

None yet

2 participants