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

add test code refs #126 #291

Draft
wants to merge 10 commits into
base: devel
Choose a base branch
from
Draft

add test code refs #126 #291

wants to merge 10 commits into from

Conversation

HugoNip
Copy link

@HugoNip HugoNip commented Mar 27, 2020

test QuantificationUtils.C, FaultTreeUtils.C

@HugoNip
Copy link
Author

HugoNip commented Mar 27, 2020

Hello, @somu15 , I deleted the banned keywords and pulled this new request, but it still fails. Would you like to show me the error messages at this time? Thank you.

@HugoNip
Copy link
Author

HugoNip commented Mar 27, 2020

@somu15 , I tested this code in my PC, it works, why I still get the errors when I pull a request? Thank you.

@somu15
Copy link
Contributor

somu15 commented Mar 27, 2020

Please see the messages below. Are you referencing the right issue in your commits?

ERROR: Your patch does not contain a valid ticket reference! (i.e. #1234)
remove the banned keywords
add new test code

ERROR: The following files contain banned keywords (std::cout, std::cerr, sleep, print_trace):
include/utils/QuantificationUtils.h
src/utils/FaultTreeUtils.C
src/utils/QuantificationUtils.C

@somu15
Copy link
Contributor

somu15 commented Mar 27, 2020

Also, if you'd like to know where your tests are failing, click on the failed check and login to CIVET using github (top).

@cbolisetti
Copy link
Contributor

cbolisetti commented Mar 27, 2020

@HugoNip thanks for this hard work. A few comments.

  • MASTODON and MOOSE PRs check for more than just compilation errors (formatting, for example). So even if your compilation was successful locally, your PR may fail. Also, that doesn't mean that you need to close the PR and open a new one. You can just add another commit fixing your code according to the comments and push it to your branch and the PR will be updated automatically.

  • While @somu15 is correct in saying that we prefer fewer commits, currently, please focus on getting the PR to pass. We can fix the commit history later.

  • Also, as @somu15 mentioned, you can see where the PR errors out if you click on details and login with your GitHub credentials. I might have to add you to the MASTODON team first, so I'll do that now.

  • You don't need to define FaultTreeUtils, QuantificationUtils, etc., as separate classes. You can just add the methods in the various namespaces. We can talk more about this on a call next week.

  • As @somu15 mentioned, you have to reference an issue in your commit message. Usually, with the commit message. In your case, say something like "Added new test code. Refs Create Fault Tree Analysis Vectorpostprocessor #126" where 126 is the issue that is addressed in this PR.

  • Looks like another error is about using banned keywords. You cannot use them even in comments for the PR to pass. This is again an error about formatting and submitting clean code.

  • Lastly, a failed PR is not a bad thing at all. We submit PRs that are work in progress all the time.

@HugoNip
Copy link
Author

HugoNip commented Mar 27, 2020

@cbolisetti Hi, Chandu, thank you for your reply. As you say, I do use the banned keywords in comments because these parts are the printing related functions that are in the original c++ code. I think that's why the PR failed. I will remove them and try it later. Thank you very much.

@@ -0,0 +1,8 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this json file.

#include "FaultTreeUtils.h"
#include "MastodonUtils.h"

#define CLIP(A, MIN, MAX) (((A) < MIN) ? MIN : ((A) > MAX ? MAX : (A)))
Copy link
Contributor

Choose a reason for hiding this comment

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

please make this a method.


#define CLIP(A, MIN, MAX) (((A) < MIN) ? MIN : ((A) > MAX ? MAX : (A)))

#define GEN_QUALIFICATION_R_VEC(gen, d, n, rv) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a method as well.

p_max = (p_max > (vec.size() - 1)) ? vec.size() : p_max;
return vec[p_min - 1] + ((p_i - p_min) * (vec[p_max - 1] - vec[p_min - 1]));
}
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

this commented section is failing the PR because of the cout.

{"NORM", NORM},
{"LNORM", LNORM},
{"PE", PE},
/*{"CHI_SQ" , CHI_SQ },*/
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove comments

FTAUtils::FaultTree::getRoot()
/*!endpublic*/ { return _root; }

/*!public*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all the commented sections, eventually.

HugoNip added 4 commits March 28, 2020 22:36
2. remove Utils.h
3. make macro #define a method
4. remove all commented sections
5. remove banned keywords
1. make macro #define a method
2. remove all commented sections
3. remove banned keywords
1. make macro #define a method
2. remove all commented sections
3. remove banned keywords
@cbolisetti
Copy link
Contributor

@HugoNip please follow the directions in the above comment and that will make style changes automatically. You should then push that commit.

@moosebuild
Copy link

Job Precheck on 128b3d0 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/mastodon/docs/PRs/291/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 71bfd3f0b6369812bcfaa5af2c0833a611f4b1a5

@moosebuild
Copy link

Job Test on a94027a wanted to post the following:

View the site here

This comment will be updated on new commits.

@HugoNip HugoNip linked an issue Mar 31, 2020 that may be closed by this pull request
@cbolisetti cbolisetti marked this pull request as draft July 14, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Fault Tree Analysis Vectorpostprocessor
4 participants