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

Fix master #1682

Merged
merged 4 commits into from Jul 9, 2020
Merged

Fix master #1682

merged 4 commits into from Jul 9, 2020

Conversation

samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jul 4, 2020

Trying to fix master.

@codecov
Copy link

@codecov codecov bot commented Jul 4, 2020

Codecov Report

Merging #1682 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1682      +/-   ##
==========================================
+ Coverage   99.68%   99.94%   +0.25%     
==========================================
  Files          21       21              
  Lines        3863     3863              
  Branches      767      767              
==========================================
+ Hits         3851     3861      +10     
+ Misses         10        1       -9     
+ Partials        2        1       -1     
Impacted Files Coverage Δ
pydantic/fields.py 100.00% <ø> (ø)
pydantic/env_settings.py 100.00% <100.00%> (+2.59%) ⬆️
pydantic/types.py 99.57% <0.00%> (ø)
pydantic/networks.py 100.00% <0.00%> (+0.77%) ⬆️
pydantic/utils.py 100.00% <0.00%> (+0.89%) ⬆️
pydantic/typing.py 100.00% <0.00%> (+1.88%) ⬆️
pydantic/version.py 100.00% <0.00%> (+11.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dac6764...f035da3. Read the comment docs.

@samuelcolvin samuelcolvin mentioned this pull request Jul 4, 2020
4 tasks
@Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Jul 4, 2020

validate_always is getting set to True causing error because of 8aebba2#diff-1ef405a6b5a2fa9f20ea7e1f92e4c38eR510 combined with dca9855#diff-1ef405a6b5a2fa9f20ea7e1f92e4c38eR468

@samuelcolvin samuelcolvin mentioned this pull request Jul 4, 2020
5 tasks
Kilo59
Kilo59 approved these changes Jul 4, 2020
@Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Jul 5, 2020

The strange part is that tests were passing on windows somehow. This potentially means that pydantic could behave differently on this platform

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 6, 2020

@samuelcolvin Do you want some help to investigate this matter?

@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Jul 6, 2020

This is preventing v1.6 from being released.

Any help on what's going wrong would be useful, I'll try and look later today.

@Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Jul 6, 2020

I see this as a plan of investigating and fixing the issue:

  • Find out why tests were broken — done in #1682 (comment) and #1682 (comment)
  • Find out why this changes didn't break tests on windows #1682 (comment)
  • Restore expected behaviour while maintaining features introduced in #1545 and #1627
  • Fix detecting failing running tests on CI on windows

Unfortunately, I don't have IDE installed on windows to investigate, if someone could help with that, it would be great.

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 6, 2020

So the real problem is that we have a "regression" with #1545 where

def test_conlist():
    class Model(BaseModel):
        v: conlist(int) = 'a'

    m = Model()
    assert m.v == 'a'

used to be valid with 1.5.1 because validate_always would not be set to True but now it does.
Do we actually need the always=True in list validator (same for recently added set) ?

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 7, 2020

Meh I just tried on a windows 10 and python 3.8.2:

tests\test_types.py:249 (test_constrained_set_default)
def test_constrained_set_default():
        class ConSetModelMax(BaseModel):
            v: conset(int) = {}
    
>       m = ConSetModelMax()

Would be nice to be able to connect via SSH on the windows 2019 server machine of github

@Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Jul 7, 2020

Well, I looked at CI output closer and noticed that tests actually failed, but CI did not detect it for some reason:
(and yeah, test_env_file_custom_encoding fails on windows ever since it was introduced)

================================== FAILURES ===================================
________________________ test_env_file_custom_encoding ________________________

tmp_path = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_env_file_custom_encoding0')

    @pytest.mark.skipif(not dotenv, reason='python-dotenv not installed')
    def test_env_file_custom_encoding(tmp_path):
        p = tmp_path / '.env'
        p.write_text('pika=p!�@', encoding='latin-1')
    
        class Settings(BaseSettings):
            pika: str
    
        with pytest.raises(UnicodeDecodeError):
>           Settings(_env_file=str(p))
E           Failed: DID NOT RAISE <class 'UnicodeDecodeError'>

tests\test_settings.py:557: Failed

________________________ test_constrained_set_default _________________________

    def test_constrained_set_default():
        class ConSetModelMax(BaseModel):
            v: conset(int) = {}
    
>       m = ConSetModelMax()

tests\test_types.py:254: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

__pydantic_self__ = ConSetModelMax(), data = {}, values = {}, fields_set = set()
validation_error = ValidationError(model='ConSetModelMax', errors=[{'loc': ('v',), 'msg': 'value is not a valid set', 'type': 'type_error.set'}])

    def __init__(__pydantic_self__, **data: Any) -> None:
        """
        Create a new model by parsing and validating input data from keyword arguments.
    
        Raises ValidationError if the input data cannot be parsed to form a valid model.
        """
        # Uses something other than `self` the first arg to allow "self" as a settable attribute
        if TYPE_CHECKING:
            __pydantic_self__.__dict__: Dict[str, Any] = {}
            __pydantic_self__.__fields_set__: 'SetStr' = set()
        values, fields_set, validation_error = validate_model(__pydantic_self__.__class__, data)
        if validation_error:
>           raise validation_error
E           pydantic.error_wrappers.ValidationError: 1 validation error for ConSetModelMax
E           v
E             value is not a valid set (type=type_error.set)

pydantic\main.py:346: ValidationError

=========================== short test summary info ===========================
FAILED tests/test_settings.py::test_env_file_custom_encoding - Failed: DID NO...
FAILED tests/test_types.py::test_constrained_set_default - pydantic.error_wra...
================= 2 failed, 1707 passed, 9 skipped in 27.38s ==================
mingw32-make: *** [Makefile:42: test] Error 1

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 7, 2020

Thanks for checking @MrMrRobat. It's reassuring! I was afraid it was ok only on some windows systems.
So the next question is "How do we fix the CI on windows to actually fail?"
@samuelcolvin @MrMrRobat #1694 is a proposal to fix tests on windows and avoid potential regressions. There is still the issue of the CI that does not fail on windows even though tests fail

@PrettyWood PrettyWood mentioned this pull request Jul 7, 2020
4 tasks
@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Jul 7, 2020

this is really weird, I've done quite a lot of digging but can't find anyone else having this problem with steps passing when make commands fail.

I think it must be some weird way gnu make and powershell are not working correctly together, but I can't find any clues about what's wrong.

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 7, 2020

@samuelcolvin Don't you think we could ask someone from github (actions) to have a look ? Nothing has changed on pydantic side regarding the CI. The exit code is 1 and fails on local. And we are talking about make, which is used like in every CI and has been stable for so long. I don't know if something has changed on github action side though. Maybe a new windows OS or a new binary for gnu make ? I don't think you can connect via SSH on the machine to have a deeper look (like on CircleCI) but if you can maybe it's worth a try too?

@Kilo59
Copy link
Contributor

@Kilo59 Kilo59 commented Jul 7, 2020

@samuelcolvin @PrettyWood
Could pydantic move away from make and start using invoke?
Then we don't have to rely on gnu make at all.
http://www.pyinvoke.org/

Some examples from another project I work on.
https://github.com/ExpDev07/coronavirus-tracker-api/blob/master/tasks.py

@PrettyWood
Copy link
Collaborator

@PrettyWood PrettyWood commented Jul 7, 2020

Thanks for sharing @Kilo59 (and kudos to you for this project!). But I don't think the issue is to know if make should be used or not but first to understand why the CI fails. Samuel said it could come from make but nothing is sure! And anyway if there is an issue here, it might well happen on other projects so it could be nice to dig into it and understand it. There are indeed a lot of projects that use make (personally I use it in all my python projects!)

PrettyWood and others added 3 commits Jul 9, 2020
* test: add regression test with wrong type default

* fix: remove always on conlist and conset

* fix: use utf8 as default encoding on all OS
@samuelcolvin samuelcolvin merged commit a913c37 into master Jul 9, 2020
15 checks passed
@samuelcolvin samuelcolvin deleted the fix-master branch Jul 9, 2020
@samuelcolvin
Copy link
Owner Author

@samuelcolvin samuelcolvin commented Jul 9, 2020

@MrMrRobat @PrettyWood thank you both for your help. I'm working on the windows problem now on #1700

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.

None yet

4 participants