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

doc: adding estimated execution time #41142

Merged
merged 4 commits into from Dec 17, 2021
Merged

doc: adding estimated execution time #41142

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b18d86
doc: adding estimated execution time
mawaregetsuka Dec 11, 2021
48ae57d
fixup! doc: adding estimated execution time
DerekNonGeneric Dec 11, 2021
6c6957f
Update doc/guides/contributing/pull-requests.md
Trott Dec 16, 2021
fcf00da
Update doc/guides/contributing/pull-requests.md
Trott Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion doc/guides/contributing/pull-requests.md
View file
Open in desktop
Expand Up @@ -251,7 +251,9 @@ And on Windows:
> vcbuild test
```

(See the [running tests][] section of Building guide for more details.)
(This code might take more than an hour to execute on a PC, want to run a part
of tests quickly or for more details see the [running tests][] section of
Building guide.)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(This code might take more than an hour to execute on a PC, want to run a part
of tests quickly or for more details see the [running tests][] section of
Building guide.)
Running tests can take a long time. To run a subset of the test suite, see the
[running tests][] section of the Building guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you there is no need for too much explanation here. I used the -j4 argument at run time, but it still takes more than an hour.So I thought it might be a good idea to add a numerical estimated time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please tell me what you think and finish the PR
@Trott

Copy link
Member

@Trott Trott Dec 15, 2021 •
edited

Choose a reason for hiding this comment

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

I don't know what's the typical experience (and I'm not sure that anyone else does either) so I'm inclined to not try to specify a time and go with the suggestion I made above. However, if we think it would be helpful to include a running time, maybe something like this?: "Depending on your hardware, running tests might take an hour or more."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I understand that time is hardware dependent, but based on my tests I don't think I can achieve results in less than an hour without a specialized compilation computer.
"Depending on your hardware, running tests might take an hour or more."maybe a good answer

Copy link
Member

Choose a reason for hiding this comment

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

On my Windows PC, it takes less than 15 minutes to compile and test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I exceeded more than an hour on both my Windows PC and Linux laptops and the laptop took a little longer. Maybe my hardware is too old?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about using build time in CI as the standard?

Copy link
Member

Choose a reason for hiding this comment

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

I think the text as it currently stands sufficiently covers things:

For some configurations, running all tests might take a long time (an hour or
more). To run a subset of the test suite, see the [running tests][] section of
the Building guide.

If that works for you, this can land. We have all the right approvals and CI is green.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I think the current version is pretty good


### Step 7: Push

Expand Down