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

#2621 custom tag map #2640

Closed
wants to merge 11 commits into from
Closed

Conversation

elfalem
Copy link

@elfalem elfalem commented Jul 22, 2017

Pull request for resolving #2621

@mention-bot
Copy link

@elfalem, thanks for your PR! By analyzing the history of the files in this pull request, we identified @geigerzaehler, @sampsyo and @pkess to be potential reviewers.

@elfalem
Copy link
Author

elfalem commented Jul 22, 2017

I'm not sure what's going on with the tests failing with the error AttributeError: 'ReadWriteTestBase' object has no attribute 'extension. Running the entire suite of tests locally succeeds. But running those tests individually shows the same errors as Travis. This happens for my branch as well as the current master branch.

@sampsyo
Copy link
Member

sampsyo commented Jul 23, 2017

Hmm; that’s strange! But the current commit on master seems to have succeeded on Travis? I don’t see any obvious indication about what’s going wrong.

@elfalem
Copy link
Author

elfalem commented Jul 23, 2017

I ran the tests again at different levels as follows:
one of the tests with errors from ReadWriteTestBase by itself
nosetests test.test_mediafile:ReadWriteTestBase.test_write_counters_without_total
all the tests in the class
nosetests test.test_mediafile:ReadWriteTestBase
the entire file
nosetests test.test_mediafile
the entire test suite
nosetests

I did this for the current PR branch and master and got:

Level master this PR
single test error error
test class error error
entire file success error
entire suite success success

I was able to narrow down the issue to the change I made in the following lines which was intended to avoid user configuration from affecting test runs.
https://github.com/elfalem/beets/blob/54b27e35c536691e792ca63e76c8683d534de495/test/test_mediafile.py#L301
https://github.com/elfalem/beets/blob/54b27e35c536691e792ca63e76c8683d534de495/test/test_mediafile.py#L406
https://github.com/elfalem/beets/blob/54b27e35c536691e792ca63e76c8683d534de495/test/test_mediafile.py#L411

I can take out those changes since they're not necessary for this PR and make this branch pass in Travis but I think there's a bigger issue of the same tests returning different results depending the level they're run.

@elfalem
Copy link
Author

elfalem commented Aug 11, 2017

What's the best way to proceed here?

@arcresu
Copy link
Member

arcresu commented May 19, 2019

@elfalem just a heads up that we're planning to split MediaFile out into a standalone Python package and git repository (see #3237). Your PR will obviously be affected by that, but your branch had already fallen out of date. Do you still have plans to work on these changes? If so it might be best to start off with the changes to MediaFile and then as a second stage look at using them in beets.

@elfalem
Copy link
Author

elfalem commented May 21, 2019

Thanks for the update. It has been a while since I looked at this PR so it will require significant updates regardless

@arcresu arcresu added this to the MediaFile++ milestone Jun 3, 2019
@jtpavlock
Copy link
Contributor

jtpavlock commented Jul 10, 2020

@elfalem any chance you still have interest in working on this?

@elfalem
Copy link
Author

elfalem commented Jul 12, 2020

@jtpavlock Unfortunately I do not.

@jtpavlock
Copy link
Contributor

@elfalem no worries, maybe your work will help out someone that tries to tackle this problem in the future. I'm going to close this since it would need some serious refactoring at this point as well.

@jtpavlock jtpavlock closed this Jul 12, 2020
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

5 participants