Skip to content
This repository has been archived by the owner on May 31, 2020. It is now read-only.

union and intersection frozen set #730

Closed
wants to merge 3 commits into from
Closed

Conversation

mdmjg
Copy link
Contributor

@mdmjg mdmjg commented Mar 13, 2018

These are the union and intersection methods for frozen set.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

This implementation looks great; now we need some tests to go with it (otherwise we have no guarantee that the code will keep working!)

Batavia tests take the form of samples of Python code that will be executed in both CPython and Batavia; the test case then compares the output of both runs to check that they're the same.

If you look in the datatype tests, test methods with calls to assertCodeExecution are what you're looking for. Add some tests like those to test frozenSet behavior, and this PR should be good to go!

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looking good; one small tweak needed on the test cases, and we should be good to merge!

y = frozetset([3,4,7,9])
z = 5
print(x.union(y))
print(x.union(z))
Copy link
Member

Choose a reason for hiding this comment

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

This is a good test, and it probably passes, but it's not the right way to test it in Batavia.

Batavia tests are based on comparing output of two program runs; and, to do that, it's necessary for both programs to terminate successfully, not raise an error.

If you're testing behavior that you know will (and should) raise an error, your test code should catch the exception and print a "raised an error" message; that way, the comparison has something to match up.

@mdmjg
Copy link
Contributor Author

mdmjg commented Mar 28, 2018

I made some changes but there is an important issue that could be addressed in order to make these tests pass. When you do the following in CPython:
x = frozenset([5,1,7,6])
print(x)
It will return frozenset({1, 5, 6, 7}) <- an ordered version of the frozen set.

However, when you do the same in Batavia, the program will not return the set in increasing order. This makes the union function return a frozen set that is not increasing order, which in turn makes the tests fail. In order to make the union function return exactly the same as CPython, changes unrelated to the union method would have to be implemented.

@martica
Copy link
Contributor

martica commented Jun 2, 2019

CPython isn't quite as predictable in its ordering.

These tests could be made to pass by changing print(frozenset... to print(sorted(frozenset... if the sorted() builtin was implemented.

Barring that, it looks like changing the format to something that tests properties of the output set would cover this.

Like:

x = frozenset([2,4,5,6])
y = frozenset([3,4,7,9])
z = x.union(y)
print(len(z))
print([elem in z for elem in x])
print([elem in z for elem in y])

x = frozenset([2,4,5,6])
y = frozenset([3,4,7,9])
z = x.intersection(y)
print(len(z))
print([elem in x and elem in y for elem in z])

@phildini
Copy link
Member

Hi there! It looks like this PR might be dead, so we're closing it for now. Feel free to re-open it if you'd like to continue, or think about directing your efforts to https://github.com/beeware/briefcase or https://github.com/beeware/toga. Both of these have more active development right now. 😄

@phildini phildini closed this Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants