-
Notifications
You must be signed in to change notification settings - Fork 166
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
Add job to test configure #1914
Comments
we could probably pull that off with a sharedlibs docker container. What would we be testing for? exit code or is there something else we can use to confirm success? if you can express it in bash that would be handy too. |
^ fwiw we do a bit of TAP output with bash in some of our tests that confirm various build properties, like when we compile with a shared openssl library we run |
I'm not sure we can really do it. Sure, we can test for --ninja, but why not other flag combinations? It's not possible to cover everything and people using specific combinations usually find problems soon enough. |
Yeap, the key is to test frequently used script. Should we do a poll in Node.js collaborators ? |
So, I'm sympathetic to the unpleasant surprise that @gengjiawen Are you offering to implement this or hoping to convince someone else to do it? This issue seems a duplicate of nodejs/node#29415, which was hopefully seen by build wg members (I certainly saw it, as well as |
I don't mind adding a new job just to test config flags if someone wants to put in the effort to write some bash that will run them and assert something, printing tap. It's not a bad thing to test, just low priority so consider this issue a TODO for someone with the time and interest eh? |
Added |
If I am reading the code correctly, it looks like there is already a test target |
@patrickhousley That tests the stuff in tools/ or in test/tools.... how is that related to #1914 (comment) ? Its |
Sorry, just getting started with the project here. The tests in |
Am I over thinking this maybe? Should the test be as simple as running |
I think just test |
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made. |
close in favor of nodejs/node#29415. |
Related: nodejs/node#29414.
Just to make sure configure works as expected, no need to build.
Such as
cc @targos
The text was updated successfully, but these errors were encountered: