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

Team 5 Homework 1 Submission #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jazwu
Copy link

@jazwu jazwu commented Oct 1, 2024

No description provided.

@jazwu jazwu changed the title Team 5 Homework 1 Team 5 Homework 1 Submission Oct 1, 2024
@kyotov
Copy link

kyotov commented Oct 6, 2024

this looks good!
couple of comments --

  • it is a small change, why have 4 commits? maybe squash them into one to make reviewing easier.
  • include a link to the circleci run that shows the tests running
  • did you try to run the rest of the tests in ninja (there are some perf tests for example)...

@qianxichen233 qianxichen233 force-pushed the circleci-project-setup branch from 6a50e39 to 86d20c4 Compare October 6, 2024 15:36
@qianxichen233
Copy link
Collaborator

Here is the link for our successful circleci build with default ninja unit test running: https://app.circleci.com/pipelines/circleci/89RxQ8gXukBs6pXwF2XEFo/LBJg2NqgUMKxynB5gJyqDg/10/workflows/6d9b62c4-3a0b-4dac-bc70-a799bca5e251

For the third comment, I didn't notice there is perftest before since it is not mentioned in ninja's README. I just tried to run these perftest and most of them are able to run, but there is one perftest that requires some kind of command line argument (depfile_parser_perftest) and I am not very sure what to pass to it since there doesn't seem to be any open documentation for it, nor there is any existing scripts that run the perftest automatically. Besides, I also saw some fuzzing test related stuff under misc folder, and again there doesn't seem to be any instruction on how to run it. I guess I'll try to figure it out later

@kyotov
Copy link

kyotov commented Oct 6, 2024

cool -- seeing what binaries it builds can help figure out what tests are there.
thanks for the link on CI -- it is interesting that it took 15 min to build -- sounds long?
let's see what others got.

@qianxichen233
Copy link
Collaborator

yeah, the 15 mins are mostly from clang-tidy checks. If I disable clang-tidy, it can build much more faster. Like I am not very what clang-tidy checks we want to enable for shadowdash

@kyotov
Copy link

kyotov commented Oct 6, 2024

ah, that makes sense.
clangtidy can be slow indeed.
we should probably split it in a separate pipeline eventually.

@kyotov
Copy link

kyotov commented Oct 9, 2024

one thing you can do is break a test on purpose and see how that shows up

@kyotov
Copy link

kyotov commented Oct 9, 2024

seems like more integration is needed on the tests side?
image

@qianxichen233
Copy link
Collaborator

seems like more integration is needed on the tests side? image

I am not very sure what this is. I don't have any test_sample.py in the repo, looks like this is something CircleCI automatically created in some place

@kyotov
Copy link

kyotov commented Oct 9, 2024 via email

@qianxichen233
Copy link
Collaborator

I figured out what the depfile_parser_perftest is doing (the perftest that expects some command line argument). It is a bench test for parsing dependency files (.d file). I didn't find any existing example .d file in ninja, so do we want to create our own test dependency file, or we just ignore this perftest?

Copy link

@kyotov kyotov left a comment

Choose a reason for hiding this comment

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

review and address all comments and let me know when it is ready for another look

executors:
docker-executor:
docker:
- image: cimg/base:stable
Copy link

Choose a reason for hiding this comment

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

you can build your own image for extra credit, but this is ok for this hw

Comment on lines 39 to 46
./build-cmake/ninja_test
./build-cmake/build_log_perftest
./build-cmake/canon_perftest
./build-cmake/clparser_perftest
./build-cmake/depfile_parser_perftest
./build-cmake/elide_middle_perftest
./build-cmake/hash_collision_bench
./build-cmake/manifest_parser_perftest
Copy link

Choose a reason for hiding this comment

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

maybe these should be separate tests running in parallel?

Copy link

Choose a reason for hiding this comment

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

try using ctest

@kyotov
Copy link

kyotov commented Nov 7, 2024

lets also add clangformat

Shihuihuang1103 and others added 4 commits November 10, 2024 09:07
Use ctest and clang-format

Remove add_executable and add_test

Add other tests to ctest

Fix failed tests

Fix test dir

Adjust tests: depfile_parser, manifest_parser

Set up docker image, use ctest, clang-tidy and parallel testing
Create docker image & test parallelism
Fix no test result found

fix test results dir

Try output test results to circleci

test write to test-results

debug test results directory

debug results.xml path

Adjust docker image and use `store_test_results` in circleCI

debug cmake version issue

check cmake version

Use new base image for new cmake version to output test results to ui

Rerun with new docker image

Use `store_test_results` to upload and store test results in circleCI
Add store_test_results upload and store test results to circleCI
@Shihuihuang1103
Copy link

Hi Professor, all comments above are addressed and this should be ready for a second review. Thank you!
Here is the link to the CircleCI run: link

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.

4 participants