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

Checks full API listings and archives more test results. No AUTO cherry-pick #2614

Merged
merged 4 commits into from Mar 11, 2022

Conversation

ncordon
Copy link
Contributor

@ncordon ncordon commented Mar 8, 2022

What

Checks full API listings and archives more test results.

Why

There's a file called full/src/main/resources/extended.txt that contains the procedures and functions included in apoc full. It's important this file is up to date in the PRs cause it will be used to package apoc, and if not updated, the apoc.help method indicate something is in core while it's not (cause it uses that extended.txt file internally).

It is bad practice as that file is kept up to date by a test: ./gradlew test --tests HelpExtendedTest, but that's a different problem.

This PR ensures we archive all tests from all projects but also that the aforementioned file is kept up to date.

@ncordon ncordon changed the title Improves the CI Checks full API listings and archives more test results Mar 9, 2022
@ncordon ncordon added 5.0 and removed enhancement labels Mar 9, 2022
@ncordon ncordon added the team-cypher-surface Cypher Surface team should review the PR label Mar 9, 2022
@ncordon ncordon changed the title Checks full API listings and archives more test results Checks full API listings and archives more test results. No AUTO cherry-pick Mar 9, 2022
Comment on lines +38 to +39
- name: Check procedures included in apoc full
run: git diff --exit-code full/src/main/resources/extended.txt || (echo "extended.txt file does not contain all changes" && exit -1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The build re-generates the extended.txt file. If it contains changes with respect to the one the repo contains, the build will tell us it is incomplete and the changes we have to make (via git diff)

Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

Few minor things to take into consideration but I think they should not block you from merging really, so will approve

Comment on lines -57 to -60
apoc.load.directory.async.add
apoc.load.directory.async.list
apoc.load.directory.async.remove
apoc.load.directory.async.removeAll
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it isn't really related to your work, but these procedures seems to have been added a year ago. They exists in all branches up to 4.4. But in dev they are suddenly gone without deprecation... Maybe we can make a card to investigate if this was intentional?

apoc.uuid.list
apoc.uuid.remove
apoc.uuid.removeAll
apoc.data.email
Copy link
Contributor

Choose a reason for hiding this comment

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

Note sure if it matters for when this is re-generated, but everything above this line seems to be in alphabetical order, the rest is not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but that's the order the build generates in, we cannot do much about that, although I don't have an explanation for that, but seems to be a stable order

- name: Archive test results
uses: actions/upload-artifact@v2
if: ${{ always() }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I googled to understand the meaning of this, and Stack overflow tells me it can probably be simplified to just
if: always()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this line to archive the test results also in case of test failures

@ncordon ncordon merged commit e2b1ef2 into dev Mar 11, 2022
@ncordon ncordon deleted the adds-more-checks-ci branch March 11, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 team-cypher-surface Cypher Surface team should review the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants