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

initial modularisation of 'cpdBootstrap' #38

Merged

Conversation

j-emberton
Copy link
Member

initial modularisation of 'cpdBootstrap'

Currently breaks regression tests - unknown why

Added pytest-xdist to parallelise running of tests (currently set to 1 in pyproject.toml)

@j-emberton j-emberton self-assigned this Oct 14, 2024
@j-emberton
Copy link
Member Author

I've had an initial look at modularising and testing cpdBootstrap.

I think the use of varargin in some functions is breaking the regression tests somehow. The problem is that 'Stats2' can no longer be interrogated properly. However all unit tests pass.

Would appreciate a 2nd pair of eyes on it.

@tztsai
Copy link

tztsai commented Oct 17, 2024

I've had an initial look at modularising and testing cpdBootstrap.

I think the use of varargin in some functions is breaking the regression tests somehow. The problem is that 'Stats2' can no longer be interrogated properly. However all unit tests pass.

Would appreciate a 2nd pair of eyes on it.

It turns out that the output Stats2 is a double string '"..."', so after json decoding it's still a string.

It's because in setup_Stats.m you added

if size(varargin)>0;
    Stats = jsonencode(Stats);
end

And in the cpdBootstrap....m file there is also another jsonencode if size(varargin) > 0 .

@dorchard
Copy link
Member

Hm my tests are failing with a lot of Undefined function 'XXX' for input arguments of type 'int64' (or `double').

@j-emberton
Copy link
Member Author

Still tests failing after your commit @tztsai , it's just different tests now. Did you run the test suite before you committed your change?

image

@tztsai
Copy link

tztsai commented Oct 17, 2024

The tests were passed on my machine
image

@j-emberton
Copy link
Member Author

I run 58 tests, whereas you have only run 52. Have you synced all tests locally?

@tztsai
Copy link

tztsai commented Oct 17, 2024

It's up to date with origin. But I only ran the unit tests. Now there are 58 tests.
image

@j-emberton
Copy link
Member Author

It's up to date with origin. But I only ran the unit tests. Now there are 58 tests. image

Do they all pass?

@tztsai
Copy link

tztsai commented Oct 17, 2024

Some of them failed because of this error. It's due to the getfield function and seems unrelated to Stats. Others are due to the extra input arguments to setup_Stats in the test, so I restored the varargin in setup_Stats. Current only the dot indexing errors are left.
image

@j-emberton
Copy link
Member Author

Some of them failed because of this error. It's due to the getfield function and seems unrelated to Stats. Others are due to the extra input arguments to setup_Stats in the test, so I restored the varargin in setup_Stats. Current only the dot indexing errors are left. image

Have you looked at the cpdAssign function? There's a section in it where it parses the stats2 data to extract certain fields. I think that's where it's failing.

tztsai
tztsai previously requested changes Oct 17, 2024
oneflux_steps/ustar_cp_refactor_wip/launch.m Outdated Show resolved Hide resolved
@j-emberton
Copy link
Member Author

@dorchard , did you say you had written some comments on this PR. I don't see any here.

tests/conftest.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
oneflux_steps/ustar_cp_refactor_wip/launch.m Outdated Show resolved Hide resolved
@dorchard
Copy link
Member

@dorchard , did you say you had written some comments on this PR. I don't see any here.

Sorry forgot to submit the review. Done now.

@j-emberton j-emberton marked this pull request as ready for review October 21, 2024 10:11
@j-emberton
Copy link
Member Author

all tests pass

@dorchard
Copy link
Member

I think we can merge this now.

Copy link

@tztsai tztsai left a comment

Choose a reason for hiding this comment

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

LGTM

@dorchard dorchard merged commit 1e4f989 into 29-generate-unit-tests-for-cpdbootstrap Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants