Skip to content

Update Tests with fetchers to use assumeTrue() #12803

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
Open
jnchin314 opened this issue Mar 23, 2025 · 10 comments · May be fixed by #12816
Open

Update Tests with fetchers to use assumeTrue() #12803

jnchin314 opened this issue Mar 23, 2025 · 10 comments · May be fixed by #12816
Assignees
@jnchin314
Labels
📍 Assigned Assigned by assign-issue-action (or manually assigned) 📌 Pinned

Comments

@jnchin314
Copy link
Contributor

jnchin314 commented Mar 23, 2025 •
edited
Loading

Is your suggestion for improvement related to a problem? Please describe.
When running the tests, many fetchers would fail, which might make it difficult to sift out when actual tests are failing for valid reasons. It would be better to only run fetcher tests with the assumption they are able to actually fetch information.

Describe the solution you'd like
@koppor described using assumeTrue() to help ignore some of the tests that would fail due to acceptable fetch errors.

Additional context
Maybe something like this would be better to add for tests involving fetchers.

        List<BibEntry> response = null;
        try {
            response = grobidService.processPDF(file, importFormatPreferences);
        } catch (UnknownHostException unknownHostException){
            assumeTrue(false, "Unable to fetch: Ignoring test");
        }

https://devdocs.jabref.org/code-howtos/fetchers.html
@koppor
Copy link
Member

koppor commented Mar 23, 2025

@jnchin314 Good that you picked this up. However, assumeTrue should be the first statement and check for availability of the fetcher key. - and not in the middle of the code somewhere.

@jnchin314
Copy link
Contributor Author

jnchin314 commented Mar 24, 2025 •
edited
Loading

What about using this annotation for checking for keys available? Its repeatable and I can apply it to classes or methods

@EnabledIfEnvironmentVariable(named = "AstrophysicsDataSystemAPIKey", matches = ".*")

@koppor
Copy link
Member

koppor commented Mar 24, 2025

I didn't know about this. Sounds great!

@jnchin314
Copy link
Contributor Author

I noticed that the BuildInfo.java has default values for some of the keys. Should we just try and use these keys for the test fetchers?

@jnchin314
Copy link
Contributor Author

/assign-me

@github-actions github-actions bot added the 📍 Assigned Assigned by assign-issue-action (or manually assigned) label Mar 24, 2025
@github-actions GitHub Actions
Copy link
Contributor

👋 Hey @jnchin314, thank you for your interest in this issue! 🎉

We're excited to have you on board. Start by exploring our Contributing guidelines, and don't forget to check out our workspace setup guidelines to get started smoothly.

In case you encounter failing tests during development, please check our developer FAQs!

Having any questions or issues? Feel free to ask here on GitHub. Need help setting up your local workspace? Join the conversation on JabRef's Gitter chat. And don't hesitate to open a (draft) pull request early on to show the direction it is heading towards. This way, you will receive valuable feedback.

Happy coding! 🚀

⏳ Please note, you will be automatically unassigned if there is not a (draft) pull request within 14 days (by 07 April 2025).

@jnchin314 jnchin314 linked a pull request Mar 24, 2025 that will close this issue
Open
2 tasks
@jnchin314
Copy link
Contributor Author

#12816

@jnchin314
Copy link
Contributor Author

This only helped a bit in reducing the number of failed tests. I think there are other reasons tests fail (AKA hosts failling, or 401/403 issues). Unfortunately those issues seem out of scope to fixing missing api keys.

@jnchin314
Copy link
Contributor Author

jnchin314 commented Mar 25, 2025 •
edited
Loading

Unfortunately the PR is now showing fetcher tests failures. Perhaps trying to use the BuildInfo.java default keys was not a good idea?

@koppor
Copy link
Member

koppor commented Mar 28, 2025

I noticed that the BuildInfo.java has default values for some of the keys. Should we just try and use these keys for the test fetchers?

Yes. Its better.

Note that these keys mostly won't work, becaus they have a very strong limit - see https://devdocs.jabref.org/code-howtos/fetchers.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jnchin314 jnchin314

Labels
📍 Assigned Assigned by assign-issue-action (or manually assigned) 📌 Pinned
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added api keys from default build file jnchin314/jabref
2 participants
@koppor @jnchin314