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

Nested jars tests #130

Closed
wants to merge 3 commits into from
Closed

Nested jars tests #130

wants to merge 3 commits into from

Conversation

ilbinek
Copy link
Collaborator

@ilbinek ilbinek commented Sep 8, 2021

@judovana
Copy link
Owner

judovana commented Sep 8, 2021

Why is the nbin.zip added, removed and again added?

Can the zipbe created in runtime pls? In pre-class?

@ilbinek
Copy link
Collaborator Author

ilbinek commented Sep 8, 2021

Why is the nbin.zip added, removed and again added?

Can the zipbe created in runtime pls? In pre-class?

Do you mean the .zip file used for the tests?

@AurumTheEnd
Copy link
Collaborator

AurumTheEnd commented Sep 8, 2021

Why is the nbin.zip added, removed and again added?
Can the zipbe created in runtime pls? In pre-class?

Do you mean the .zip file used for the tests?

Yes, the tester.zip file is weirdly added in 8572ac9, removed in e721a7c, and then re-added in 460278d.

If it is a crucial file for the tests (which I presume it is) and wish to keep it, please rebase it so that the file is added only once.

Actually, looking deeper into it, I presume that 460278d requires the tester.zip file to have different contents than before. In that case, please have the tester.zip file change in 460278d and drop the unnecessary commit e721a7c.

@judovana
Copy link
Owner

judovana commented Sep 8, 2021

..or generate it on the fly in pre-class...

@ilbinek
Copy link
Collaborator Author

ilbinek commented Sep 8, 2021

Why is the nbin.zip added, removed and again added?
Can the zipbe created in runtime pls? In pre-class?

Do you mean the .zip file used for the tests?

Yes, the tester.zip file is weirdly added in 8572ac9, removed in e721a7c, and then re-added in 460278d.

If it is a crucial file for the tests (which I presume it is) and wish to keep it, please rebase it so that the file is added only once.

Actually, looking deeper into it, I presume that 460278d requires the tester.zip file to have different contents than before. In that case, please have the tester.zip file change in 460278d and drop the unnecessary commit e721a7c.

8572ac9 Added the zip file.

e721a7c Removed one file in the zip that was not supposed to be inside and is used for testing packing.

460278d Renamed the classes inside the zip so it better represents where they are, as discussed previously.

e721a7c and 460278d are only changing a small amount of bytes (-169 and +54).

@AurumTheEnd
Copy link
Collaborator

AurumTheEnd commented Sep 8, 2021

8572ac9 Added the zip file.

e721a7c Removed one file in the zip that was not supposed to be inside and is used for testing packing.

Ah - misleading commit message. Both Jirka and I interpreted "Removed testing file" as if you had removed the whole zip file, and unfortunately GitHub's UI did not help with distinguishing that you had just "Removed one leftover file from testing archive".

In any case, you are fixing something you yourself introduced in this PR, which has no reason for being a separate commit. Therefore please rebase it so that 8572ac9 already has the correct zip archive, and force-push.

@judovana
Copy link
Owner

and generate that zip in runtime...

@AurumTheEnd
Copy link
Collaborator

and generate that zip in runtime...

Agreed.

@ilbinek
Copy link
Collaborator Author

ilbinek commented Sep 10, 2021

and generate that zip in runtime...

Agreed.

Ok, will do

@AurumTheEnd
Copy link
Collaborator

Just as a side-note, did you have to take extra steps in order to manually stage and commit the tester.zip archive? Because our .gitignore clearly forbids zips. Curious.

@AurumTheEnd AurumTheEnd linked an issue Sep 16, 2021 that may be closed by this pull request
@ilbinek
Copy link
Collaborator Author

ilbinek commented Oct 6, 2021

Closing PR, will open new one with rewritten implementation.

@ilbinek ilbinek closed this Oct 6, 2021
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.

Nested jars: unit tests
3 participants