Skip to content

Conversation

@cssherman
Copy link
Collaborator

@cssherman cssherman commented Jan 23, 2024

This PR does the following:

  1. Uses a separate test directory (the location of the ats files), working directory (location where the tests are run), and baseline directory.
  2. Uses the test directory structure for the working and baseline directories
  3. Removes a bunch of unused/broken code in TestCase
  4. Removes unused report types
  5. Re-works the report base class to use modern ATS approach (removing a bunch of unnecessary/broken manual checks)
  6. Replaces manual html table construction with the tabulate package
  7. Updates the style/format of the html documentation

I'm going to work on branches of GEOS/integratedTests that implement this version next

GEOS-DEV/GEOS#2964

Copy link
Contributor

@TotoGaz TotoGaz left a comment

Choose a reason for hiding this comment

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

So much code removed!
This would require testing of course. Is it OK on the CI?

Comment on lines 18 to 29
COLORS = {}
COLORS[EXPECTED.name] = "black"
COLORS[CREATED.name] = "black"
COLORS[BATCHED.name] = "black"
COLORS[FILTERED.name] = "black"
COLORS[SKIPPED.name] = "orange"
COLORS[RUNNING.name] = "blue"
COLORS[PASSED.name] = "green"
COLORS[TIMEDOUT.name] = "red"
COLORS[HALTED.name] = "brown"
COLORS[LSFERROR.name] = "brown"
COLORS[FAILED.name] = "red"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COLORS = {}
COLORS[EXPECTED.name] = "black"
COLORS[CREATED.name] = "black"
COLORS[BATCHED.name] = "black"
COLORS[FILTERED.name] = "black"
COLORS[SKIPPED.name] = "orange"
COLORS[RUNNING.name] = "blue"
COLORS[PASSED.name] = "green"
COLORS[TIMEDOUT.name] = "red"
COLORS[HALTED.name] = "brown"
COLORS[LSFERROR.name] = "brown"
COLORS[FAILED.name] = "red"
COLORS: Mapping[str, str] = {
EXPECTED.name: "black",
CREATED.name: "black",
BATCHED.name: "black",
FILTERED.name: "black",
SKIPPED.name: "orange",
RUNNING.name: "blue",
PASSED.name: "green",
TIMEDOUT.name: "red",
HALTED.name: "brown",
LSFERROR.name: "brown",
FAILED.name: "red",
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely, and I'm sure there is more. I ended up using this package to help identify unused code: https://pypi.org/project/vulture/

Comment on lines 49 to 50
action_names = ','.join(action_options.keys())
parser.add_argument("-a", "--action", type=str, default="run", help=f"Test actions options ({action_names})")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use parser.add_argument(...., choices=action_options.keys(), ...) which would make the check and the doc +/- automatic? Same for check_options and verbose_options but I cannot find those in the parser 🤣


atsargv.extend(ats_files)
atsargv.append(options.ats_target)
sys.argv = atsargv
Copy link
Contributor

Choose a reason for hiding this comment

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

Is sys.argv = atsargv even legal?? 😲

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was actually one of the less weird things that our original version of ats did... We're essentially intercepting the command line args, doing some custom work, then creating new args for ats.


# Setup paths
ats_root_dir = os.path.abspath(os.path.dirname(options.ats_target))
os.chdir(ats_root_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have to do this chdir? I've seen that was done like that in the previous code, but it sucks so much!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that I've changed enough of the code to use absolute paths at this point... I'll give this a shot

@cssherman cssherman force-pushed the feature/sherman/outOfPlaceATS branch from 66dfd30 to a3608d0 Compare January 29, 2024 20:17


def generate_geos_tests( decks: Iterable[ TestDeck ] ):
def generate_geos_tests( decks: Iterable[ TestDeck ], test_type='smoke' ):
Copy link
Collaborator

Choose a reason for hiding this comment

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

great!

@cssherman cssherman merged commit d543c04 into main Feb 13, 2024
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.

4 participants