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

test[next]: simplify boilerplate for fieldview single feature tests #1236

Merged
merged 44 commits into from
May 2, 2023
Merged

test[next]: simplify boilerplate for fieldview single feature tests #1236

merged 44 commits into from
May 2, 2023

Conversation

DropD
Copy link
Contributor

@DropD DropD commented Apr 12, 2023 •
edited
Loading

Description

Added:

  • next_tests.feature_tests.cases
    • helpers to express repetitive test setup concisely

Changed:

  • next_tests.feature_tests.ffront_tests.test_execution
    • several test methods use the "cases" helpers

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

@DropD DropD requested a review from tehrengruber April 12, 2023 14:27
@DropD DropD marked this pull request as ready for review April 19, 2023 15:39
@DropD DropD requested review from egparedes and havogt April 19, 2023 15:39
havogt
havogt requested changes Apr 21, 2023
View reviewed changes
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Show resolved Hide resolved
egparedes
egparedes requested changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

I have some comments and questions.

tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tehrengruber
tehrengruber requested changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

Partial review only.

tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
Rico Häuselmann added 16 commits April 25, 2023 15:05
fix allocate and default verify scalar arg tests
18e6eab
extended docstrings for allocate and verify
0f523ab
spellcheck the docstrings
41a104a
use get_type_hints from eve.extended_typing
dfaa775
fix rst markup in docstrings
069a193
use ScalarValue type alias everywhere appropriate
9e0ba4f
unignore cases from mypy, fix typing, fix tests
641a004
explicitly import reduction_setup fixture in cases
fec2f50
Merge remote-tracking branch 'upstream/main' into c14-feature-test-setup
b4409dd
re-exclude cartesian tests from mypy in pre-commit
798ae73
Merge remote-tracking branch 'upstream/main' into c14-feature-test-setup
60a43fe
remove support for multidim scalars
3ecb224
cleanup superfluous type ignore
818bfe2
tests cleanup
b88d646
match testee arg naming convention to ref lambdas
332a44f
fix tests
67608c9
tehrengruber
tehrengruber approved these changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

Only small things left.

tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/ffront_tests/test_execution.py Outdated Show resolved Hide resolved
@DropD DropD requested review from egparedes and havogt April 27, 2023 18:11
egparedes
egparedes approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks good to me. I have some (optional) suggestions and a couple of small docstring and typing fixes, but I don't need a new review cycle for them so I approve the PR now.

src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Outdated Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
tests/next_tests/integration_tests/feature_tests/cases.py Show resolved Hide resolved
havogt
havogt approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

I didn't do another round of review, but it already got a lot of feedback. Let's merge.

@DropD DropD merged commit ba907d9 into GridTools:main May 2, 2023
@DropD DropD deleted the c14-feature-test-setup branch May 2, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egparedes egparedes egparedes approved these changes

@havogt havogt havogt approved these changes

@tehrengruber tehrengruber tehrengruber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DropD @egparedes @havogt @tehrengruber