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.

Sign up for GitHub

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

Jump to bottom

Initial ComStock CI #90

Merged
merged 4 commits into from
Jan 24, 2024
Merged

Initial ComStock CI #90

merged 4 commits into from
Jan 24, 2024

Conversation

wenyikuang
Copy link
Collaborator

Initial the measure test from ruby.
call the cbci_shared_libs's building_comstock_all when the PR is merged.

  • Fixes #ISSUENUMBERHERE (IF RELEVANT)

Pull Request Author

This pull request makes changes to (select all the apply):

  • Documentation
  • Infrastructure (includes singularity image, buildstock batch, dependencies, continuous integration tests)
  • Sampling
  • Workflow Measures
  • Upgrade Measures
  • Reporting Measures
  • Postprocessing

Author pull request checklist:

  • Tagged the pull request with the appropriate label (documentation, infrastructure, sampling, workflow measure, upgrade measure, reporting measure, postprocessing) to help categorize changes in the release notes.
  • Added tests for new measures
  • Updated measure .xml(s)
  • Register values added to comstock_column_definitions.csv
  • Both options_lookup.tsv files updated
  • 10k+ test run
  • Change documentation written
  • Measure documentation written
  • ComStock documentation updated
  • Changes reflected in example .yml files
  • Changes reflected in README.md files
  • Added 'See ComStock License' language to first two lines of each code file
  • All new and existing tests pass the CI

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a code review on GitHub
  • All related changes have been implemented: data and method additions, changes, tests
  • If fixing a defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • Reviewed change documentation
  • Ensured code files contain License reference
  • Results differences are reasonable
  • CI status: all tests pass

ComStock Licensing Language - Add to Beginning of Each Code File

# ComStock™, Copyright (c) 2023 Alliance for Sustainable Energy, LLC. All rights reserved.
# See top level LICENSE.txt file for license terms.

TODO:

@wenyikuang wenyikuang changed the title Wenyi/ci pr initial the comstock ci infra Jan 10, 2024
@wenyikuang wenyikuang assigned wenyikuang and unassigned wenyikuang Jan 10, 2024
@wenyikuang wenyikuang marked this pull request as draft January 10, 2024 04:46
@wenyikuang
Copy link
Collaborator Author

wenyikuang commented Jan 10, 2024 •
edited
Loading

Link to the real script of ci:

https://github.com/NREL/cbci_jenkins_libs/blob/develop/vars/building_comstock_all.groovy

asparke2
asparke2 reviewed Jan 18, 2024
View reviewed changes
Copy link
Member

@asparke2 asparke2 left a comment

Choose a reason for hiding this comment

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

Few comments, but not much, looks good to me.

Rakefile Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
test/helpers/minitest_helper.rb Outdated Show resolved Hide resolved
resources/measures/ChangeBuildingLocation/tests/change_building_location_test.rb
@@ -38,6 +38,7 @@

require 'openstudio'
require 'openstudio/measure/ShowRunnerOutput'
require_relative '../../../../test/helpers/minitest_helper'
require 'fileutils'
Copy link
Member

Choose a reason for hiding this comment

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

This file is missing require 'minitest/autorun', which is in the other measure test files. I think since minitest_helper.rb contains require 'minitest/autorun' it should be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I set all the test related library imported and configurations in the minitest_helper to keep it simper.

asparke2
asparke2 requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@asparke2 asparke2 left a comment

Choose a reason for hiding this comment

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

Minor comment to remove some comments

@wenyikuang
Copy link
Collaborator Author

Minor comment to remove some comments

@asparke2 Great! Let me clean them up! Thanks for your time!

@wenyikuang wenyikuang marked this pull request as ready for review January 24, 2024 03:56
@wenyikuang
Copy link
Collaborator Author

the recent changes I made in my pull request. I conducted tests involving path changes and Jenkins cooperation, and the results can be reviewed in the commit here on a temporary branch (which will be removed once approved).

You can check the detailed Jenkins pipeline execution and results here. Notably, the Ruby tests are now being read correctly.

Thanks,
Wenyi

@asparke2 asparke2 self-assigned this Jan 24, 2024
@asparke2
Copy link
Member

It doesn't seem like the PR status indicators on GitHub (below) match the status indicators shown on Jenkins:
image

  1. The command Archive the artifacts is failing with a red X in run measure tests but the ruby-tests workflow has a green check mark in the Jenkins pipeline diagram. Seems like a failure to write the test artifacts should make run measure tests fail because without the test results XML, the failing tests aren't shown. It looks like the test/report.xml artifact is from a python test. Maybe /test/report2.xml in ruby was to avoid overwriting the python test output file?
    image

  2. I don't see any test failures for "run measure tests," probably because the step Archive the artifacts is failing?
    image

  3. The command bundle config... rake unit_test:resource_measure_tests is failing with a red X in run resources measure tests, but that step has a yellow exclamation point in the Jenkins workflow diagram. It seems like the Jenkins pipeline should show the worst status (red X). At least the Tests page in Jenkins correctly shows the failing resource_measure_tests
    image

asparke2
asparke2 requested changes Jan 24, 2024
View reviewed changes
Copy link
Member

@asparke2 asparke2 left a comment

Choose a reason for hiding this comment

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

The workflow status indicators on Jenkins still don't match what is shown on GitHub, I think that needs to be fixed before merging.

@asparke2
Copy link
Member

@wenyikuang add checkbox to GitHub PR template to add measure test to test list .txt file

@wenyikuang
Copy link
Collaborator Author

Part of the issues are caused by I removed most of the tests from the test list, therefore not much results get recorded.

part of the issues are caused by I stop the running of CI earlier.
I rerun it this morning, and here is the full result

wenyikuang
wenyikuang commented Jan 24, 2024
View reviewed changes
Jenkinsfile_main_develop
@@ -0,0 +1,9 @@
#!/usr/bin/groovy

@Library('cbci_shared_libs@develop') _
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep it as develop for now, once we are getting used to it remove the @develop branch tag.

@asparke2 asparke2 merged commit 8d57897 into main Jan 24, 2024
@asparke2 asparke2 deleted the wenyi/ci-PR branch January 24, 2024 21:09
@mdahlhausen mdahlhausen changed the title initial the comstock ci infra Initial ComStock CI Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asparke2 asparke2 asparke2 approved these changes

@rHorsey rHorsey Awaiting requested review from rHorsey

Assignees

@asparke2 asparke2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wenyikuang @asparke2 @rHorsey