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

Add mesh reader test for trusses #28

Closed
wants to merge 10 commits into from
Closed

Conversation

jtveiten
Copy link
Contributor

The mesh part is now read. Need to do the model part later, keyword density isn't recognized yet

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cd3e8e0 on jtveiten:master into a9677c7 on JuliaFEM:master.

Copy link
Collaborator

@ovainola ovainola left a comment

Choose a reason for hiding this comment

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

Issues discussed in chat must be addressed

Copy link
Collaborator

@ovainola ovainola left a comment

Choose a reason for hiding this comment

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

et22sfse.inp file is copyrighted by Dassault Systems, file needs to be fetch from external location

ahojukka5 and others added 2 commits March 11, 2018 17:15
Function can be used to download test models from internet. In order to
use function, one must set environment variable ABAQUS_DOWNLOAD_URL.
@ahojukka5
Copy link
Member

ahojukka5 commented Mar 11, 2018

We are not sure if it is ok to add that test file into the repository, it probably is copyrighted by Dassault. I implemented a new function download_abaqus which can be used to download files on the fly before testing, requiring that one has set ABAQUS_DOWNLOAD_URL to point someplace where the files can be downloaded. See PR #29, and here.

Could you do the following changes:

  • rebase from newest master to get download_abaqus
  • remove file et22sfse.inp from repository
  • use fn = abaqus_download("et22sfse.inp") to download the file during the testing. fn will be the path for the downloaded file.

If you want to do local testing, you must do something like ENV["ABAQUS_DOWNLOAD_URL"] = "https://my_models.com/v6.14/books/eif" before using the function, otherwise, it returns nothing.

Also, consider squashing changes to keep version history clean. This also prevents the file et22sfse.inp from being written into the repository at any point. Here is a quite good explanation how squashing works.

@ahojukka5
Copy link
Member

Looks that my test is breaking further tests. I have to fix it.

@jtveiten
Copy link
Contributor Author

First of all, apologies for not looking at the terms and conditions.
I have done the changes, but I wasn't sure I got the squashing right after some trys, so I haven't done that yet.
The test runs locally on my machine as far as I can see?

Return environment variables to the original state after finish test.
@ahojukka5
Copy link
Member

It was another test messing up environment variables and not returning the original state. If you now rebase again for newest master this should work.

@jtveiten
Copy link
Contributor Author

jtveiten commented Mar 11, 2018

If I run it locally after setting the env. variable, including the reader and including runtests.jl, it seems to succeed. But if do an include("runtests.jl") again, it fails on my machine

New argument introduced, env=ENV.
@ahojukka5
Copy link
Member

One more time, rebase for the newest master.

@ahojukka5 ahojukka5 closed this Mar 11, 2018
@ahojukka5 ahojukka5 reopened this Mar 11, 2018
@ahojukka5
Copy link
Member

@jtveiten
Copy link
Contributor Author

jtveiten commented Mar 11, 2018

Do I need to protect the test by checking for an empty string returned from abaqus_download?
This will work once it the code is in master? Or a different approach on reistering filenames?

@ahojukka5
Copy link
Member

ahojukka5 commented Mar 11, 2018

Well the test probably should work, if branch is under JuliaFEM/AbaqusReader.jl, let see what happens when I opened a new PR based on this change. PR #32.

@jtveiten
Copy link
Contributor Author

So that worked. But it is unfortunate that the forks don't build.

@ahojukka5
Copy link
Member

Ok this is now merged in PR #32.

@ahojukka5 ahojukka5 closed this Mar 11, 2018
@ahojukka5
Copy link
Member

Yes, but there is a good reason why environment variables are not set for pull requests coming outside of the organization. Someone could do something like println(ENV["PASSWORD"]) and then read the output from Travis CI log...

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

4 participants