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

Fixed FunctionalTestGetsnAllTrue by creating non-static APP_XML #4022

Merged
merged 2 commits into from
Sep 17, 2020
Merged

Fixed FunctionalTestGetsnAllTrue by creating non-static APP_XML #4022

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
395f53c
Fixed FunctionalTestGetsnAllTrue by creating non-static APP_XML
Sep 17, 2020
784f9fa
Replaced static APP_XML with per-test instances
Sep 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions isis/tests/FunctionalTestsGetsn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ static QString APP_XML = FileName("$ISISROOT/bin/xml/getsn.xml").expanded();

// check for all correct outputs
TEST_F(DefaultCube, FunctionalTestGetsnAllTrue) {
QString d_APP_XML = FileName("$ISISROOT/bin/xml/getsn.xml").expanded();
Copy link
Contributor

Choose a reason for hiding this comment

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

If static variables are an issue in test cases, we should get rid of the static variable and switch all of the test cases over to using this. Another option would be to sub-class the DefaultCube and put the app xml into the fixture.

Copy link
Collaborator

@Kelvinrr Kelvinrr Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand how a static var could cause the issue though? It should make it local to the file, if anything being non static global would cause the behavior where the var is leaking into other tests. Even within the same test module, the variable isn't being changed. The post Austin linked to talked about static classes which could have statefulness across tests, but that's not what is happening here.

And why does this test need the var changed but the other do not?

Lastly, putting it into the fixture would get messy especially if a set of app tests rely on different fixtures.

I'm wondering if using something like #DEFINE GETSN_XML_PATH would work as an alternative to defining the string once but reusing it in a way that can't cause the var to be accidentally manipulated in any way.

QString expectedSN = "Viking1/VISB/33322515";
QString expectedON = "Viking1/VISB/33322515";
QVector<QString> args = {
"FILE=TRUE",
QVector<QString> args = { "FILE=TRUE",
"SN=TRUE",
"OBSERVATION=TRUE"};
UserInterface options(APP_XML, args);
UserInterface options(d_APP_XML, args);
Pvl appLog;

getsn( testCube, options, &appLog );
PvlGroup results = appLog.findGroup("Results");
PvlGroup results = appLog.findGroup("Results");

EXPECT_PRED_FORMAT2(AssertQStringsEqual, results.findKeyword("Filename"), testCube->fileName());
EXPECT_PRED_FORMAT2(AssertQStringsEqual, results.findKeyword("SerialNumber"), expectedSN);
EXPECT_PRED_FORMAT2(AssertQStringsEqual, results.findKeyword("ObservationNumber"), expectedON);
Expand Down Expand Up @@ -66,9 +66,9 @@ TEST_F(DefaultCube, FunctionalTestGetsnDefaultTrue) {
Pvl appLog;
Pvl *testLabel = testCube->label();
testLabel->findObject( "IsisCube" ).deleteGroup( "Instrument" );

getsn( testCube, options, &appLog );
PvlGroup results = appLog.findGroup("Results");
PvlGroup results = appLog.findGroup("Results");

EXPECT_PRED_FORMAT2(AssertQStringsEqual, fileName , results.findKeyword("SerialNumber"));
}
Expand All @@ -83,9 +83,9 @@ TEST_F(DefaultCube, FunctionalTestGetsnDefaultFalse) {
Pvl appLog;
Pvl *testLabel = testCube->label();
testLabel->findObject( "IsisCube" ).deleteGroup( "Instrument" );

getsn( testCube, options, &appLog );
PvlGroup results = appLog.findGroup("Results");
PvlGroup results = appLog.findGroup("Results");

EXPECT_PRED_FORMAT2(AssertQStringsEqual, fileName , results.findKeyword("SerialNumber"));
}
Expand Down Expand Up @@ -114,7 +114,7 @@ TEST_F(DefaultCube, FunctionalTestGetsnFlat) {
// Test that append true appends to file
TEST_F(DefaultCube, FunctionalTestGetsnAppend) {
QFile flatFile(tempDir.path()+"testOut.txt");
QVector<QString> args = {
QVector<QString> args = {
"FORMAT=FLAT",
"TO="+flatFile.fileName(),
"APPEND=TRUE"};
Expand All @@ -133,7 +133,7 @@ TEST_F(DefaultCube, FunctionalTestGetsnAppend) {
// Test that append false overwrites file
TEST_F(DefaultCube, FunctionalTestGetsnOverwrite) {
QFile flatFile(tempDir.path()+"testOut.txt");
QVector<QString> args = {
QVector<QString> args = {
"FORMAT=FLAT",
"TO="+flatFile.fileName(),
"APPEND=FALSE"};
Expand Down